Skip to content

(RSP-1523) Fixing close button positioning for dismissible dialogs#250

Merged
LFDanLu merged 3 commits into
masterfrom
RSP-1523
Mar 9, 2020
Merged

(RSP-1523) Fixing close button positioning for dismissible dialogs#250
LFDanLu merged 3 commits into
masterfrom
RSP-1523

Conversation

@LFDanLu
Copy link
Copy Markdown
Member

@LFDanLu LFDanLu commented Mar 5, 2020

Closes https://jira.corp.adobe.com/browse/RSP-1523

✅ Pull Request Checklist:

  • Included link to corresponding Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Go to the "isDismissible" and "with hero, isDismissible" Dialog stories in storybook and confirm that the 'X' button is 12px/8px from the edge of the dialog in medium/large scale respectively. Try RTL locale as well.

Comment on lines -190 to -196
.spectrum-Dialog--dismissable {
.spectrum-Dialog-closeButton {
display: block;
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Unsure why this existed in the first place, but it was causing the focus ring to be slightly off center. Lemme know if we needed this

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might need to verify with @snowystinger for this one

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this was just here because closeButton was display none unless the dialog was dismissable
however, i think i changed it to be a js solution instead, so you should be fine removing this
i don't know if css will like this change though?

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

Copy link
Copy Markdown
Contributor

@MidnightCoder06 MidnightCoder06 left a comment

Choose a reason for hiding this comment

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

Visually lgtm. Assuming that bit of css is not needed, issue seems fixed 👍

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@adobe-bot
Copy link
Copy Markdown

Build successful! 🎉

@LFDanLu LFDanLu merged commit 0e9e0d3 into master Mar 9, 2020
@LFDanLu LFDanLu deleted the RSP-1523 branch March 9, 2020 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants