[#2201] Toast Component added with locales #2210
Conversation
|
Thank you for opening this pull request with us! Be sure to follow our Pull Request Practices. Let us know if you have any questions on Slack. |
There was a problem hiding this comment.
Thanks, @zeeshansarwar38 for contributing to if-me.
I think you haven't worked on this case when we log out, which means when we log out, this alert is shown there also.
Please made changes according to this too.
There was a problem hiding this comment.
https://www.awesomescreenshot.com/image/35214908?key=2f4ad31c0b5c17ed2ce45258526bc1f9
@zeeshansarwar38 screenshot of Log out screen
@MuhammadAakash have you tested these changes by pulling the code from my branch? I checked this on my local environment and it works fine for the logout scenario. |
|
@zeeshansarwar38, Sure I will check this and let you know. |
There was a problem hiding this comment.
Thanks, @zeeshansarwar38.
Everything is working fine.
@julianguyen if there are no other changes required from your side then this branch is ready to merge.
|
|
||
| .clr-white { | ||
| color: $white; | ||
| } |
There was a problem hiding this comment.
Can we remove this class? I don't see it being used anywhere.
There was a problem hiding this comment.
Acknowledged
| }, 7000); | ||
| } | ||
| return ( | ||
| <div> |
There was a problem hiding this comment.
We can use a Fragment here i.e. <> and </>.
There was a problem hiding this comment.
We were using that fragment already but I didn't comprehend your last comment on this change so I converted that to a div, assuming that's what you want :)
| } | ||
| return ( | ||
| <div> | ||
| <div id="toast-notice" aria-label={showNotice ? I18n.t('alert_auto_hide') : ''} style={{ visibility: showNotice ? 'visible' : 'hidden' }} role="region" aria-live="polite" aria-atomic="true" className={`${showNotice ? 'notice' : ''} ${css.toast} ${showNotice && (showAlert || appendDashboardClass) ? 'smallMarginBottom' : ''}`}> |
There was a problem hiding this comment.
Let's not do inline CSS here and use stylesheets via SCSS.
There was a problem hiding this comment.
Acknowledged
| } | ||
| return ( | ||
| <div> | ||
| <div id="toast-notice" aria-label={showNotice ? I18n.t('alert_auto_hide') : ''} style={{ visibility: showNotice ? 'visible' : 'hidden' }} role="region" aria-live="polite" aria-atomic="true" className={`${showNotice ? 'notice' : ''} ${css.toast} ${showNotice && (showAlert || appendDashboardClass) ? 'smallMarginBottom' : ''}`}> |
There was a problem hiding this comment.
I think you'll want to only render this entire element only if showNotice exists.
There was a problem hiding this comment.
Actually, we need to have these two divs #toast-notice and #toast-alert present in the DOM all the time because we are using aria-live and role=alert for screen-reader users. So it is a requirement that the element should be present in the page markup first. Please check these links for reference link link-2.
| </> | ||
| )} | ||
| </div> | ||
| <div id="toast-alert" aria-label={showAlert ? I18n.t('alert_auto_hide') : ''} style={{ visibility: showAlert ? 'visible' : 'hidden' }} role="alert" aria-live="assertive" aria-atomic="true" className={`${showAlert ? 'alert' : ''} ${css.toast} ${showAlert && appendDashboardClass ? 'smallMarginBottom' : ''}`}> |
There was a problem hiding this comment.
You'll want to only render this entire element only if showAlert exists.
There was a problem hiding this comment.
Actually, we need to have these two divs #toast-notice and #toast-alert present in the DOM all the time because we are using aria-live and role=alert for screen-reader users. So it is a requirement that the element should be present in the page markup first. Please check these links for reference link link-2.
|
Thank you for merging this pull request with us! If you haven't already, in another pull request, please add yourself to our About page. |
Description
Corresponding Issue
(#2201)
Screenshots
Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!