-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
A11y design enhancements #6563
Merged
ndelangen
merged 23 commits into
storybookjs:next
from
CodeByAlex:a11y-design-enhancements
Apr 25, 2019
Merged
A11y design enhancements #6563
ndelangen
merged 23 commits into
storybookjs:next
from
CodeByAlex:a11y-design-enhancements
Apr 25, 2019
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
…eckbox when addon panel is in the vertical position. This is similar to to the effect of media queries but media queries only work for window resizing not element resizing.
…nel (horizontally or vertically) is of a certain width.
This pull request is automatically deployed with Now. Latest deployment for this branch: https://monorepo-git-fork-codebyalex-a11y-design-e.storybook.now.sh |
CodeByAlex
requested review from
jbovenschen,
ndelangen,
shilman and
tmeasday
as code owners
April 18, 2019 19:44
Wow! Looks good! Firstly, I think need to fix conflicts. |
Armanio
requested changes
Apr 18, 2019
Shoot I removed that magic number constant when I was testing something.
Nice catch! I’ll make sure to add that back in. I originally had media
queries but media queries aren’t responsive when the addon panel is
shortened in width in the vertical view.
…On Thu, Apr 18, 2019 at 4:07 PM Arman ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In addons/a11y/src/components/Report/Rules.tsx
<#6563 (comment)>:
> fontWeight: '400',
} as any);
-const Item = styled.div({
- display: 'flex',
- flexDirection: 'row',
- marginBottom: '6px',
-});
+const Item = styled.div(({ elementWidth }: { elementWidth: number }) => ({
+ flexDirection: elementWidth > 407 ? 'row' : 'inherit',
hmm, the magic number
why 407? what is it?
------------------------------
In addons/a11y/src/components/Report/Rules.tsx
<#6563 (comment)>:
> + switch (rule.impact) {
+ case ImpactValue.CRITICAL:
+ badgeType = 'critical';
+ break;
+ case ImpactValue.SERIOUS:
+ badgeType = 'negative';
+ break;
+ case ImpactValue.MODERATE:
+ badgeType = 'warning';
+ break;
+ case ImpactValue.MINOR:
+ badgeType = 'neutral';
+ break;
+ default:
+ break;
+ }
⬇️ Suggested change
- }
+const badgeType = rule.impact in ImpactValue ? rule.impact : null;
can simplify?
------------------------------
In addons/a11y/src/components/Tabs.tsx
<#6563 (comment)>:
> - },
-}));
+const GlobalToggle = styled.div(({ elementWidth }: { elementWidth: number }) => {
+ const maxElementWidth = 450;
+ return {
+ cursor: 'pointer',
+ fontSize: '14px',
+ padding: elementWidth > maxElementWidth ? '12px 15px 10px 0' : '12px 0px 3px 12px',
+ height: '40px',
+ border: 'none',
+ marginTop: elementWidth > maxElementWidth ? '-40px' : '0px',
+ float: elementWidth > maxElementWidth ? 'right' : 'left',
+ display: elementWidth > maxElementWidth ? 'flex' : 'block',
+ alignItems: 'center',
+ width: elementWidth > maxElementWidth ? 'auto' : '100%',
+ borderBottom: elementWidth > maxElementWidth ? 'none' : '1px solid rgba(0,0,0,.1)',
why you didn't use media queries?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#6563 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHDCQBRRUFITHIZWYC6OPODPRDIJPANCNFSM4HG6OIUQ>
.
|
I did try to find a css solution before going with a JS solution but
nothing came up for what I needed for this use case.
On Thu, Apr 18, 2019 at 4:10 PM Alexander Wilson <
wilsonc.alexander@gmail.com> wrote:
… Shoot I removed that magic number constant when I was testing something.
Nice catch! I’ll make sure to add that back in. I originally had media
queries but media queries aren’t responsive when the addon panel is
shortened in width in the vertical view.
On Thu, Apr 18, 2019 at 4:07 PM Arman ***@***.***> wrote:
> ***@***.**** requested changes on this pull request.
> ------------------------------
>
> In addons/a11y/src/components/Report/Rules.tsx
> <#6563 (comment)>
> :
>
> > fontWeight: '400',
>
> } as any);
>
>
>
> -const Item = styled.div({
>
> - display: 'flex',
>
> - flexDirection: 'row',
>
> - marginBottom: '6px',
>
> -});
>
> +const Item = styled.div(({ elementWidth }: { elementWidth: number }) => ({
>
> + flexDirection: elementWidth > 407 ? 'row' : 'inherit',
>
>
> hmm, the magic number
> why 407? what is it?
> ------------------------------
>
> In addons/a11y/src/components/Report/Rules.tsx
> <#6563 (comment)>
> :
>
> > + switch (rule.impact) {
>
> + case ImpactValue.CRITICAL:
>
> + badgeType = 'critical';
>
> + break;
>
> + case ImpactValue.SERIOUS:
>
> + badgeType = 'negative';
>
> + break;
>
> + case ImpactValue.MODERATE:
>
> + badgeType = 'warning';
>
> + break;
>
> + case ImpactValue.MINOR:
>
> + badgeType = 'neutral';
>
> + break;
>
> + default:
>
> + break;
>
> + }
>
>
> ⬇️ Suggested change
>
> - }
>
> +const badgeType = rule.impact in ImpactValue ? rule.impact : null;
>
>
> can simplify?
> ------------------------------
>
> In addons/a11y/src/components/Tabs.tsx
> <#6563 (comment)>
> :
>
> > - },
>
> -}));
>
> +const GlobalToggle = styled.div(({ elementWidth }: { elementWidth: number }) => {
>
> + const maxElementWidth = 450;
>
> + return {
>
> + cursor: 'pointer',
>
> + fontSize: '14px',
>
> + padding: elementWidth > maxElementWidth ? '12px 15px 10px 0' : '12px 0px 3px 12px',
>
> + height: '40px',
>
> + border: 'none',
>
> + marginTop: elementWidth > maxElementWidth ? '-40px' : '0px',
>
> + float: elementWidth > maxElementWidth ? 'right' : 'left',
>
> + display: elementWidth > maxElementWidth ? 'flex' : 'block',
>
> + alignItems: 'center',
>
> + width: elementWidth > maxElementWidth ? 'auto' : '100%',
>
> + borderBottom: elementWidth > maxElementWidth ? 'none' : '1px solid rgba(0,0,0,.1)',
>
>
> why you didn't use media queries?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#6563 (review)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AHDCQBRRUFITHIZWYC6OPODPRDIJPANCNFSM4HG6OIUQ>
> .
>
|
This was referenced Apr 7, 2022
This was referenced May 13, 2022
This was referenced May 13, 2022
This was referenced May 14, 2022
This was referenced May 14, 2022
This was referenced Jun 21, 2023
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.
What I did
I have modified the X and check marks to be more accessible because the color of the X alone is difficult extract meaning from if you are visually impaired. They now are colored and list out the severity/urgency of the issue (which really provides better information anyway).
OLD:
NEW:
I also updated the text that appears when there aren't any rules within a tab. It used to use the word "A11y" and now it uses the word "accessibility". This helps people that don't understand what a11y means.
OLD:
NEW:
The global checkbox and all of the badges now respond nicely (when the addons panel is mobile, vertical, or horizontal).
OLD:
NEW:
Capitalized the first character in colorblindness emulation text
How to test
If your answer is yes to any of these, please make sure to include it in your PR.