Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Better alignment for Alert icons #807

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

frankieyan
Copy link
Member

@frankieyan frankieyan commented Oct 31, 2023

Short description

This fixes the alignment for icons in the <Alert> component, spotted by @pauloslund over at https://github.com/Doist/todoist-web/pull/8223#pullrequestreview-1706995679

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • Reviewed and approved Chromatic visual regression tests in CI

Versioning

Patch

<AlertIcon tone={tone} className={styles.icon} />
<Box display="flex" alignItems="center">
<AlertIcon tone={tone} className={styles.icon} />
</Box>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this results in a similar but different issue. On single line alerts without a close button, the icon and the content itself end up not being vertically aligned within the alert. You can see in the image below that the alignment between the two Alerts don't quite match:

image

I think part of this is caused by the min-height being calculated for the container.

I have a solution for it though! This is the diff for the changes I'd make:

diff --git a/src/alert/alert.module.css b/src/alert/alert.module.css
index d5b95121..e319e425 100644
--- a/src/alert/alert.module.css
+++ b/src/alert/alert.module.css
@@ -3,8 +3,14 @@
     border-width: 1px;
     color: var(--reactist-content-primary);
     padding: var(--reactist-spacing-small);
-    /* this is to make sure it always has the same minimum height, whether it has a close button or not */
-    min-height: calc(2 * var(--reactist-spacing-small) + var(--reactist-button-small-height) + 2px);
+}
+
+.icon {
+    display: block;
+}
+
+.content {
+    /* this is to make sure it always has the same minimum height, whether it has a close button or not */
+    min-height: var(--reactist-button-small-height);
 }

 .tone-info {
diff --git a/src/alert/alert.tsx b/src/alert/alert.tsx
index 9bf372ba..00399ac6 100644
--- a/src/alert/alert.tsx
+++ b/src/alert/alert.tsx
@@ -35,14 +35,15 @@ function Alert({ id, children, tone, closeLabel, onClose }: AlertProps) {
         >
             <Columns space="small" alignY="center">
                 <Column width="content">
-                    <Box display="flex" alignItems="center">
-                        <AlertIcon tone={tone} className={styles.icon} />
-                    </Box>
+                    <AlertIcon tone={tone} className={styles.icon} />
                 </Column>
                 <Column>
                     <Box
                         paddingY="xsmall"
                         paddingRight={onClose != null && closeLabel != null ? undefined : 'small'}
+                        display="flex"
+                        alignItems="center"
+                        className={styles.content}
                     >
                         {children}
                     </Box>

There are two fixes here:

  1. Setting the .icon to display: block; solves the icon centering issue. However, this results in the same problem of the icon and content not being centered within the alert itself.
  2. Move the minimum height to the alert content instead of the alert container.

This is what it looks like after those changes:
image

Thoughts?

@frankieyan
Copy link
Member Author

I will take care of this when I'm back after OOO!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants