"Alert" + "Toast" components - Finalize implementation in code (feature branch) #245
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Made tweaks along the way: -Change title to required, body optional - Clarify icon is set at 24
Was debugging the failed test
From looking at the other components, we don't want to include <p> (and makes sense with all the globals on <p> e.g. in cloud-ui)
Made tweaks along the way: -Change title to required, body optional - Clarify icon is set at 24
Was debugging the failed test
Dhaulagiri
reviewed
May 6, 2022
packages/components/tests/integration/components/hds/alert/index-test.js
Outdated
Show resolved
Hide resolved
MelSumner
reviewed
May 6, 2022
packages/components/tests/dummy/app/templates/components/alert.hbs
Outdated
Show resolved
Hide resolved
MelSumner
reviewed
May 6, 2022
| assert.dom('.flight-icon-alert-diamond').exists(); | ||
| }); | ||
|
|
||
| test('if an icon is declared, the icon should render in the component and override the default one', async function (assert) { |
There was a problem hiding this comment.
[suggestion] should there be a test here to ensure that the "Default one" renders?
There was a problem hiding this comment.
It's the previous test ('it should render an icon by default depending on the type and color')
MelSumner
reviewed
May 6, 2022
| assert.dom('.hds-alert__title').hasText('This is the title'); | ||
| assert.dom('.hds-alert__description').hasText('This is the description'); | ||
| }); | ||
| test('it should render rich HTML when the @description argument contains HTML tags', async function (assert) { |
There was a problem hiding this comment.
[comment] I'm surprised that we are even offering this.
There was a problem hiding this comment.
[comment] I'm surprised that we are even offering this.
we have multiple of this cases in the audit, we have to support them. and we're (more correctly, we'll be) supporting basic HTML elements like a, strong, em, code (see CSS and comments)
….hbs Co-authored-by: Melanie Sumner <melanie@hashicorp.com>
cveigt
suggested changes
May 9, 2022
MelSumner
reviewed
May 9, 2022
Co-authored-by: Christian Veigt <cveigt@gmail.com>
MelSumner
approved these changes
May 10, 2022
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📌 Summary
This is the "feature branch" used to finalize the implementation of the
AlertandToastcomponents. The implementation of these components has not followed the usual process (for [reasons]) so it's a single large PR for both the components. This PR contains the previous #56 and #91, and the follow-up changes were merged directly in this branch (for development convenience).🛠️ Detailed description
This PR contains these changes.
Alert
Alertcomponent (HTML/HBS + JS + CSS)Alertcomponent - page preview hereAlertcomponentToast
Toastcomponent (HBS + CSS) as specializedAlert(inline)instanceToastcomponent - page preview hereToastcomponent👩🔧 Things to know
Alert & Toast
At the beginning there was a single component,
Alert, that covered all the possible types of notification (Page,Inline,CompactandToast). Later in the process, it was decided to "extract" theToastin a distinct component, for many reasons (among them, it was much easier to write documentation, because these two kind of notifications have very different usages, logics, etc).The
Toastcomponent in Figma is a distinct component, in a distinct page.Similarly in code the
Toastcomponent is a distinct component, with a distinct documentation page. But under the hood is just using theHds::Alert(Inline)component (with just a couple of extra styles in CSS).APIs
There have been a few back & forth around the API for the component, in particular around using named blocks and/or contextual components (see for example this discussion: #104).
When we tested the integration of the components in Cloud UI (see below) we run into a few issues around how Ember was treating whitespace and yielded components inside named blocks, see these threads for more details:
You can see a writeup of what we did and what we learned in the process here: #104 (comment)
At the end we decided to get rid of the named blocks, and use only contextual components, to avoid (part of) these problems (and follow the same code pattern used in the dropdown).
Spike for adoption test in Cloud UI
We did a spike to test the integration of the
Hds::AlertandHds::Testcomponents in Cloud UI, to reduce as much as possible friction in the adoption phase. You can see the results of this test here: https://github.com/hashicorp/cloud-ui/pull/2429🔗 External links
👀 How to review
👉 Review by files changed (don't even try to go commit by commit :) )
Reviewer's checklist:
💬 Please consider using conventional comments when reviewing this PR.