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.

Sign up for GitHub

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

Jump to bottom

fix: alert follows fiori3 #470

Merged
merged 4 commits into from Dec 5, 2019
Merged

fix: alert follows fiori3 #470

merged 4 commits into from Dec 5, 2019

Conversation

JKMarkowski
Copy link
Contributor

Related Issue

Closes #452

Description

There is corrected styling for alert component. Now it follows latest fiori3 requirements.

Screenshots

Missing screens will be added soon

NOTE: If you've made any style changes, please provide appropriate screenshots (before and after) to help reviewers.

To see examples of which screenshots to include, go to Screenshot Examples.

Before:

After:

@netlify
Copy link

netlify bot commented Nov 28, 2019 •
edited

Deploy preview for fundamental-styles ready!

Built with commit 5dfafda

https://deploy-preview-470--fundamental-styles.netlify.com

InnaAtanasova
InnaAtanasova requested changes Nov 28, 2019
View reviewed changes
Copy link
Contributor

@InnaAtanasova InnaAtanasova left a comment

Choose a reason for hiding this comment

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

The buttons are not aligned properly

Screen Shot 2019-11-28 at 3 19 36 PM

Screen Shot 2019-11-28 at 3 17 57 PM

The info icon is not in the middle
Screen Shot 2019-11-28 at 3 19 00 PM

@droshev droshev added the Fiori 3 refactoring match Fiori 3 requirements label Nov 29, 2019
@droshev droshev added this to In progress in Development via automation Nov 29, 2019
@droshev droshev added this to the Sprint 25 - Paris milestone Nov 29, 2019
@JKMarkowski
Copy link
Contributor Author

Everything is fixed now, Icons are not aligned well, when fd-link is being used. It will be fixed automatically after merging #475

InnaAtanasova
InnaAtanasova reviewed Nov 29, 2019
View reviewed changes
src/alert.scss Outdated
@@ -12,7 +12,7 @@ $block: #{$fd-namespace}-alert;
.#{$block} {
$fd-alert-border: 1px solid;
$fd-alert-border-radius: 0.25rem;
$fd-alert-padding: 0.5rem 1rem;
$fd-alert-padding: 0.375rem 1rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the top and bottom padding should be 0.4375rem. In specs they say 0.5rem (including border)

InnaAtanasova
InnaAtanasova reviewed Nov 29, 2019
View reviewed changes
src/alert.scss Outdated
Comment on lines 28 to 36
@mixin fd-alert-close-btn-container {
position: absolute;
width: 2rem;
height: 1.625rem;
top: 0.375rem;
top: 0.125rem;
right: 0.125rem;
display: flex;
align-items: center;
justify-content: center;
min-width: 2rem;
height: 1.625rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

The interaction states of the button should follow these of the Transparent button

@droshev droshev self-requested a review December 1, 2019 18:20
Development automation moved this from In progress to Review in progress Dec 1, 2019
droshev
droshev requested changes Dec 1, 2019
View reviewed changes
Copy link
Contributor

@droshev droshev left a comment

Choose a reason for hiding this comment

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

The dismiss button color is not using the fiori 3 colors and instead is referring to the browser color.

@droshev
Copy link
Contributor

droshev commented Dec 2, 2019

@JKMarkowski the pipeline is failing

@claassistantio
Copy link

claassistantio commented Dec 4, 2019 •
edited

CLA assistant check
All committers have signed the CLA.

@claassistantio
Copy link

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ JKMarkowski
✅ InnaAtanasova
❌ Testerski

Testerski seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Development automation moved this from Review in progress to Reviewer approved Dec 4, 2019
@JKMarkowski JKMarkowski merged commit 5c5a530 into master Dec 5, 2019
Development automation moved this from Reviewer approved to Done Dec 5, 2019
@JKMarkowski JKMarkowski deleted the fix/452-alert branch December 5, 2019 08:51
@JKMarkowski JKMarkowski mentioned this pull request Dec 6, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@droshev droshev droshev approved these changes

@InnaAtanasova InnaAtanasova InnaAtanasova approved these changes

@jbadan jbadan Awaiting requested review from jbadan

@estelaxu3 estelaxu3 Awaiting requested review from estelaxu3

@stefanoScalzo stefanoScalzo Awaiting requested review from stefanoScalzo

Assignees
No one assigned
Labels
Fiori 3 refactoring match Fiori 3 requirements
Projects
Development
  
Done
Milestone
Sprint 25 - Paris
Development

Successfully merging this pull request may close these issues.

Alert doesn't match Fiori 3 specs
4 participants
@JKMarkowski @droshev @claassistantio @InnaAtanasova