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(alert, notice): update leading and close icons to follow calcite-action icon sizing #2431

Merged
merged 5 commits into from
Jul 7, 2021

Conversation

jcfranco
Copy link
Member

@jcfranco jcfranco commented Jul 2, 2021

Related Issue: #2417

Summary

This updates alert and notice's x and start icons as follows:

  • when component scale is medium => use small scale for icons
  • when component scale is large => use medium scale for icons
  • when component scale is small => no change

notice

Screen Shot 2021-07-01 at 6 24 51 PM

alert

Screen Shot 2021-07-01 at 6 27 22 PM
Screen Shot 2021-07-01 at 6 27 57 PM
Screen Shot 2021-07-01 at 6 28 12 PM

@jcfranco jcfranco added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Jul 2, 2021
@jcfranco jcfranco added this to the Sprint 6/21 – 7/2 milestone Jul 2, 2021
@jcfranco jcfranco requested a review from bstifle July 2, 2021 01:36
@jcfranco jcfranco self-assigned this Jul 2, 2021
@jcfranco jcfranco requested a review from a team as a code owner July 2, 2021 01:36
@bstifle
Copy link

bstifle commented Jul 2, 2021

image

@jcfranco looks good, but would like to see both demo pages updated with examples that have actions in them if you dont mind

small medium and large with alert and notice would be 💯

I'll approve branch once thats reviewed

@jcfranco
Copy link
Member Author

jcfranco commented Jul 2, 2021

@bstifle Unlike notice, alert doesn't have a specific slot for actions.

@bstifle
Copy link

bstifle commented Jul 2, 2021

gotcha gotcha.

i suppose we'll add those later with #2382

@bstifle
Copy link

bstifle commented Jul 2, 2021

@jcfranco looks like small actions are a bit off on the width

can we get these to match the X (close button) padding?

image

@jcfranco
Copy link
Member Author

jcfranco commented Jul 2, 2021

Sure, but that means updating calcite-action's padding for small scale globally (from sizing-2 to sizing-3). Is that OK?

@julio8a
Copy link

julio8a commented Jul 2, 2021

I think I have this in my PR for action-bar, to update the padding on action. It's the spacing between the icon and the title. Not sure if it's the same but thought I'd bring it up.

https://github.com/Esri/calcite-components/pull/2429/files#diff-4c5550b21bcb6014dcecc11ee9b564798b1231a352538340cb9fabef70917489R110

@jcfranco
Copy link
Member Author

jcfranco commented Jul 2, 2021

@julio8a Thanks for bringing it up. I tested your branch and it looks like the close buttons still need to be tweaked. I synced-up w/ @bstifle and I'll be modifying the padding on the close buttons for consistency.

Before pushing, can you confirm this is the spacing we want for the small scale?

Screen Shot 2021-07-02 at 11 01 30 AM
Screen Shot 2021-07-02 at 11 01 41 AM

@jcfranco
Copy link
Member Author

jcfranco commented Jul 6, 2021

I forgot to add these:

notice
Screen Shot 2021-07-06 at 1 46 11 PM
Screen Shot 2021-07-06 at 1 46 08 PM

alert
Screen Shot 2021-07-06 at 1 52 52 PM

@bstifle @macandcheese does this change look good? I had to tweak the small scale var that is used to pad the sides of the alert/notice to make the sizes match.

@jcfranco
Copy link
Member Author

jcfranco commented Jul 6, 2021

Synced up w/ @bstifle and rolled back my last change since it affected more than the close buttons. Updated the PR to only modify spacing for small scale close icons.

Copy link

@bstifle bstifle left a comment

Choose a reason for hiding this comment

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

lookin real good, waiting on screener

@jcfranco
Copy link
Member Author

jcfranco commented Jul 7, 2021

@bstifle I see the same spacing (aside from icons) when I compare storybook on master and this branch. Proceeding to merge based on that.

@jcfranco jcfranco merged commit d415aef into master Jul 7, 2021
@jcfranco jcfranco deleted the jcfranco/2417-update-notice-and-icon-sizing branch July 7, 2021 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug reports for broken functionality. Issues should include a reproduction of the bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants