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: [M3-7072] - Long drawer titles overlapping close icon #9731

Merged

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Sep 28, 2023

Description 📝

This PR fixes an edge case where very long Drawer titles (in this case, due to long firewall names) overlap the Drawer close (X) icon.

Note: we fixed a similar issue before with Dialogs, not Drawers.

Major Changes 🔄

List highlighting major changes

  • Adds right margin to the drawer title to prevent overlap with close icon
  • Reorders props to resolve eslint perfectionist warnings
  • Fixes a typo that appears in the component's Storybook documentation

Preview 📷

Before After
longdrawertitle_prod Screenshot 2023-10-04 at 6 56 05 AM

How to test 🧪

  1. How to setup test environment?
  • Create a firewall with a long label (max is 32 characters) at https://cloud.linode.com/firewalls/create with word breaks that get as close as possible to the close button. (e.g. test-this-re-l-y-l-o-n-gf-ir-e-w)
  • Check out this PR.
  • yarn dev
  1. How to reproduce the issue (if applicable)?
  • Go to https://cloud.linode.com/firewalls and visit the details page for your new firewall with a really long name.
  • Select the Linodes tab and click the button to Add Linodes to Firewall.
  • Observe the AddDeviceDrawer that opens and has a long title that overlaps the close icon.
  1. How to verify changes?
  • Go to http://localhost:3000/firewalls and visit the details page for your new firewall with a really long name.
  • Select the Linodes tab and click the button to Add Linodes to Firewall.
  • Observe the AddDeviceDrawer that opens and has a long title that does not overlap the close icon.
  • Check a few other drawers throughout the app to ensure there have been no unintended styling regressions from eslint reordering by comparing to prod.
  1. How to run Unit or E2E tests?
    This was a styling regression.

@mjac0bs mjac0bs added the Bug Fixes for regressions or bugs label Sep 28, 2023
@mjac0bs mjac0bs self-assigned this Sep 28, 2023
@@ -132,7 +132,6 @@ export const Drawer = (props: Props) => {
<Grid>
<IconButton
sx={{
position: 'absolute',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this property because with a position of absolute: "the element is removed from the normal document flow, and no space is created for the element in the page layout", which is why the drawer title was overlapping the icon button -- as far as it was concerned, it didn't exist. By removing that positioning, we inherit position: relative and the icon button element is part of the document flow again.

I don't see a reason to keep position: absolute here, but if we wanted to keep it, another way to address this issue would be to add some paddingRight to the title.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolute positioning is useful for the icon button because it allows us to position it within the grid slightly offset to the upper right of the title, and does not take the icon button's spacing into account in the document layout. Spacing within icon button affects the bottom margin of the drawer header and was leading to additional spacing between the header and the rest of the drawer when the icon button was included in the document flow (position: relative used).

position: relative
Screenshot 2023-10-03 at 9 01 40 PM

position: absolute
image

Comment on lines +57 to +62
width: 480,
},
drawerHeader: {
'&&': {
marginBottom: theme.spacing(2),
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing eslinting warnings.

Comment on lines +99 to +105
onClose={(event, reason) => {
if (onClose && reason !== 'backdropClick') {
onClose(event, reason);
}
}}
anchor="right"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing eslinting warnings.

@mjac0bs mjac0bs marked this pull request as ready for review September 28, 2023 20:06
@mjac0bs mjac0bs requested a review from a team as a code owner September 28, 2023 20:06
@mjac0bs mjac0bs requested review from jdamore-linode and hana-akamai and removed request for a team September 28, 2023 20:06
Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

Just recently noticed this issue too, thanks for putting up the fix! Confirmed that long drawer titles are no longer overlapping the close icon. 🎉

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Looks like some extra spacing is being added below all Drawer headers now.

develop This PR
Screenshot 2023-09-29 at 12 04 15 PM Screenshot 2023-09-29 at 12 04 32 PM

@mjac0bs mjac0bs force-pushed the M3-7072-drawer-title-overlapping-close-icon branch from df93e7e to b3caeb0 Compare October 4, 2023 13:42
@mjac0bs
Copy link
Contributor Author

mjac0bs commented Oct 4, 2023

Looks like some extra spacing is being added below all Drawer headers now.

@bnussman-akamai Should be addressed now.

I ended up reverting the change to remove absolute padding and added a right margin to the title to account for the icon button within the drawer header.

Prod This Branch
editvolume_prod editvolume_thisbranch

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Looks good throughout the app! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fixes for regressions or bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants