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

feat: add no-icon modifier for alert to match UI Kit v1.4.3 #1493

Merged
merged 7 commits into from
Jun 26, 2019

Conversation

saad-mo
Copy link
Contributor

@saad-mo saad-mo commented Jun 14, 2019

Closes #1492

adds no-icon modifier to render information, success, warning and error without an icon

Screen Shot 2019-06-14 at 10 08 44 AM

Test

Changelog

New

  • no-icon modifier

Changed

nothing

Removed

nothing

@saad-mo saad-mo requested review from a team and LeoT7508 June 14, 2019 15:30
@saad-mo saad-mo changed the title Feature/1492 no icon alert feat: add no icon modifier for alert Jun 14, 2019
@saad-mo saad-mo changed the title feat: add no icon modifier for alert feat: add no-icon modifier for alert Jun 14, 2019
@netlify
Copy link

netlify bot commented Jun 14, 2019

Deploy preview for fundamental ready!

Built with commit 0e49949

https://deploy-preview-1493--fundamental.netlify.com

Copy link

@LeoT7508 LeoT7508 left a comment

Choose a reason for hiding this comment

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

@saad-mo

Can we double check the size of the icons? They look big. The close icon should be 16px and 12px from the edge. The icons on the left should be 16px and also 12px from the edge.

@droshev
Copy link
Contributor

droshev commented Jun 19, 2019

@saad-mo can you cherry pick this in fundamental-styles too once you align with Leo?

@saad-mo
Copy link
Contributor Author

saad-mo commented Jun 24, 2019

@LeoT7508 updated the icon and close button size. it was indeed bigger.

https://deploy-preview-1493--fundamental.netlify.com/components/alert.html

Copy link

@LeoT7508 LeoT7508 left a comment

Choose a reason for hiding this comment

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

It looks like the height of these are off, looks like it might be 38px, it should be 40px tall. The close icon looks right.

@saad-mo
Copy link
Contributor Author

saad-mo commented Jun 24, 2019

It looks like the height of these are off, looks like it might be 38px, it should be 40px tall. The close icon looks right.

height on a single line alert updated to 40px - https://deploy-preview-1493--fundamental.netlify.com/components/alert.html

Copy link

@LeoT7508 LeoT7508 left a comment

Choose a reason for hiding this comment

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

It looks like the icons are now not centered vertically.

Screen Shot 2019-06-25 at 9 47 29 AM

@saad-mo saad-mo changed the title feat: add no-icon modifier for alert feat: add no-icon modifier for alert to match UI Kit v1.4.3 Jun 25, 2019
@saad-mo
Copy link
Contributor Author

saad-mo commented Jun 26, 2019

It looks like the icons are now not centered vertically.

Screen Shot 2019-06-25 at 9 47 29 AM

fixed.

Screen Shot 2019-06-26 at 2 40 30 PM

Copy link

@LeoT7508 LeoT7508 left a comment

Choose a reason for hiding this comment

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

Positioning is correct.

@saad-mo saad-mo merged commit a85e37b into master Jun 26, 2019
@saad-mo saad-mo deleted the feature/1492-no-icon-alert branch June 26, 2019 20:13
droshev pushed a commit that referenced this pull request Jul 16, 2020
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.

Lib: add no-icon modifier to alerts
3 participants