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.
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
Feature: #41 Alert. #60
Feature: #41 Alert. #60
Changes from 17 commits
cae4905
b65fad5
24dab7b
b769038
a238e1d
e9ae7f7
99be962
693c6ca
646a156
2885dd9
57ce07a
5de4065
95a531f
f756efa
78c7cb2
0400552
c6b73de
30cf5fa
9b17e1f
704e602
0e32697
532d22c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need a SB example for every possible iteration/variant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's beneficial to write stories for every visual representation of a component like we do with the button component, especially as we implement VRT. It would be good to detect if something you did broke a layout of a component in a specific state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm down to have this discussion in a larger setting, but for me SB serves t purposes. 1: To show what the component can do. 2: To show how to use the component.
I think 1 is taken care of by the controls we take the time to set up. So the number of variants in a story should be dictated by the different ways the component can be used. EX: the alertdialog option here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LeeMellon , I think that's an interesting point.
This is the first time I've used storybook, so the patterns are new to me. I can see the point of view that a smaller number of stories may be easier for an implementer since it focuses their attention.
If it's helpful, this is what I've used Storybook for so far.
I write the storybook page first with examples of the variations that I understand need to be represented. In the alert, this means one of each status type. There is some level of feeling things out here. On a current project, we have an icon that can show 1800 icons and I don't have 1800 stories. So, I think 4 statuses makes sense to me as stories, but 1800 doesn't. I made another component to show all the colors / icons and those have their own component pages. In development, I was settings CSS classes per status type, but there were no CSS changes for those icons. Meaning, it was possible for me to break a single status type, but not a single icon.
If there is JS interaction, I may write the Jest tests at this point too.
Then, I start implementing the component itself. At this time, I'm watching those stories to see the components change and gradually get the features that I expect. Most of the time I'm looking at the
docs
pane, or for interaction / screen reader theiframe
example.If I forgot an example, I'll add more stories to capture those use cases.
... And I don't really use the controls much.
I agree with @mabry1985, that for visual regression testing, some of the additional variations may be helpful since the tools will default to one screenshot of each.
Might be a good team discussion or one for Github discussions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if they're simple/obvious let's keep the standard of docs for every/state/method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree, I've been doing this and reflecting that comment in the story controls. I'd like to look into creating a method for using those JSDocs to populate the storybook control info for us. Stencil does something similar.
I think for Storybook, especially, it's important to be explicit with the comments because there are QA, design, clients, etc. who are going to be seeing these pages. Its more user friendly and accessible is my argument.
A Storybook style guide is something I'd love to get nailed down soon, I'm just trying to figure out the best way to write them still too. Would be a good discussion for guild.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably needs to be
shouldHideIcon
because of the way attributes work in the DOM. The presence of an attribute meanstrue
, so there is no way to set a false value if the default istrue
. :(So, all boolean attributes probably need to default to false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting 🤯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going to require some more 'plumbing'.
Looks like you're going to need for 'alertdialog':
https://developer.mozilla.org/enUS/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_alertdialog_role
I see the SB example, but we can't always be sure that the consumer will pass the requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the links! Maybe we can remove this feature for now and add a todo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could change
isInteractive
to something likealertDialogue: string
and if it's there pass that to an aria-label attrib, and then set the role to "alertdialog". Just a thought to keep the functionality. Also we may just need to have a focusable close button here either way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this use the
<outline-header>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the question. I'm not sure.
In the storybook examples I left it as a slot, so we could pass in an
<outline-header>
if desired.The trade-off there seems to be a little more work on the consumer of the component for the flexibility to define the header level / visual level.
I suppose we could always look to see if the slotted component was an
<outline-header>
, text, or something else and then only wrap text in the<outline-header>
. What levels would we use then as defaults?Anyone have ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I saw the
this.statusType
and was thinking that was the text that was passed, but realize that is just a placeholder and we should be using content in the slot so that might make more sense. One reason I guess would be to keep all headings the same style.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Chris on this one, I think this should be an
outline-heading
with the statusType rendered inside of it. I don't think there is much benefit to allowing a consumer to place their own element here. If someone wanted to do that they could extend this element and overwrite thelargeTemplate()
method I'd mentioned above.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a while on a project with the enhanced editor I think the slot is a better idea.