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(components): Keep dismiss on drawers aligned to top when titles wrap #2011

Merged
merged 7 commits into from
Sep 23, 2024

Conversation

fagnanm
Copy link
Contributor

@fagnanm fagnanm commented Sep 4, 2024

Motivations

When there are longer titles on drawers (like in Automations where the title is user-generated) the dismiss button tries to align center with the height of the title making things look real weird.

Changes

Adjusted the structure of the headers on drawer and sideDrawer slightly so that the titles appear center aligned but are actually aligned to the start of the container. This keeps the dismiss button and subsequent actions in the right place even if the title wraps onto more than one line.

Changed

Before

Side Drawer
image

Drawer
image

After

Side Drawer
image

Drawer
image

Testing

Try adding titles that wrap onto more than one line and ones that do not. Actions in the headers should still align center with the heading.

Changes can be
tested via Pre-release


In Atlantis we use Github's built in pull request reviews.

Random photo of Atlantis

Copy link

cloudflare-workers-and-pages bot commented Sep 4, 2024

Deploying atlantis with  Cloudflare Pages  Cloudflare Pages

Latest commit: f877b2f
Status: ✅  Deploy successful!
Preview URL: https://5b733494.atlantis.pages.dev
Branch Preview URL: https://align-sidedrawer-header-top.atlantis.pages.dev

View logs

@fagnanm fagnanm changed the title Align sidedrawer header top fix(component): Keep dismiss on drawers aligned to top when titles wrap Sep 4, 2024
@fagnanm fagnanm changed the title fix(component): Keep dismiss on drawers aligned to top when titles wrap fix(components): Keep dismiss on drawers aligned to top when titles wrap Sep 4, 2024
@@ -1,6 +1,6 @@
.container {
--drawer-width: 280px;
--drawer--base-padding: var(--space-large);
--drawer--base-padding: var(--space-base);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the padding match sideDrawer so they are the same

}

/* Make sure the dismiss button doesn't get squished when title is longer */
.header > button {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed an existing issue with the button shrinking if the title was too long

@fagnanm fagnanm marked this pull request as ready for review September 5, 2024 15:08
@fagnanm fagnanm requested a review from a team as a code owner September 5, 2024 15:08
@taylorvnoj taylorvnoj self-requested a review September 20, 2024 17:22
Copy link
Contributor

@taylorvnoj taylorvnoj left a comment

Choose a reason for hiding this comment

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

It's looking better! I did notice that with a longer word and smaller breakpoint, I'm seeing the dismiss button go beyond the page in Drawer. Top is local and bottom is current behaviour.

Screenshot 2024-09-20 at 5 20 20 PM

gap: var(--space-base);
}

.heading {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need overflow: hidden in heading or something similar?

If I have a long word, the dismiss icon goes beyond the page at a smaller breakpoint/when the drawer snaps to a smaller size.
Screenshot 2024-09-20 at 5 04 39 PM

SideDrawer seems to handle it a bit better

Screenshot 2024-09-20 at 5 16 34 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@taylorvnoj I'll see if I can fix that, sometimes limited CSS solutions to really long words when their containers are smaller.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@taylorvnoj played with a few things but I don't believe this will be fixable without some potential unpredictability in where words break. We could do something like word-break: break-word which will work in some cases but it doesn't have a predictable pattern on where it breaks the words, which is usually an accessibility concern for a screen reader.

I'd say we don't solve it given how many drawers have titles set by us. In cases where drawer titles are set by the user, I imagine it would be very uncommon to have words that exceed the width of the drawer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that's fair. I do want us at a team to think about/consider localization as well but for the reasons you mentioned I agree that we can leave this as is.

We also have many more uses of SideDrawer than Drawer in product.

Copy link

Published Pre-release for f877b2f with versions:

  - @jobber/components@5.29.1-align-side-f877b2f.8+f877b2ff

To install the new version(s) for Web run:

npm install @jobber/components@5.29.1-align-side-f877b2f.8+f877b2ff

@taylorvnoj taylorvnoj self-requested a review September 23, 2024 18:05
Copy link
Contributor

@taylorvnoj taylorvnoj left a comment

Choose a reason for hiding this comment

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

LGTM

@taylorvnoj taylorvnoj merged commit c866e97 into master Sep 23, 2024
13 checks passed
@taylorvnoj taylorvnoj deleted the align-sidedrawer-header-top branch September 23, 2024 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants