-
Notifications
You must be signed in to change notification settings - Fork 9
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
Ux 431 static testing #34
Conversation
@@ -1,5 +1,5 @@ | |||
@import "~tokens/tokens.scss"; | |||
|
|||
.story-body { | |||
padding: $space*2; | |||
padding: $space * 2; |
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.
cc @mikrotron 😄
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.
.eslintrc
Outdated
"rules": { | ||
"import/no-named-default": "off", | ||
"camelcase": "off", | ||
"import/no-extraneous-dependencies": "off", |
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 is a useful rule, when configured properly. I updated eslint-plugin-import
to the latest version and set up the packageDir
option. Was able to find a few missing dependencies in the packages.
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.
"es6": true, | ||
"browser": true, | ||
"node": true, | ||
"jest": true, |
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.
It'd be nice to restrict jest and cypress to just the files they're relevant for. Can be done later
.eslintrc
Outdated
"import/no-unresolved": "off", | ||
"jsx-a11y/label-has-for": "off", | ||
"jsx-a11y/href-no-hash": "off", | ||
"array-callback-return": "off", |
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 one seems fine too... let's revisit later
Co-Authored-By: nahumzs <nahumzs@users.noreply.github.com>
Co-Authored-By: nahumzs <nahumzs@users.noreply.github.com>
Co-Authored-By: nahumzs <nahumzs@users.noreply.github.com>
@@ -46,7 +46,11 @@ storiesOf("Stylers", module).add("Include Examples", () => ( | |||
<h4> | |||
<code>stylers.visuallyHidden</code> | |||
</h4> | |||
<InvisibleBox>👻</InvisibleBox> | |||
<InvisibleBox> | |||
<span role="img" aria-label="ghost"> |
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.
Is this to appease the linter?
It's just storybook code, so it's fine, but my understanding was that unicode emojis are fairly accessible on their own. The screen reader should describe them, literally. This approach makes sense when the icon represents an idea or metaphor, but in this case the experience is probably diminished for any non-anglo users (at least if their OS is not in English).
Not an argument to change it, just wondering.
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.
yeap, a11y-linter, complains about it, I just want to make it happy :D ...
@@ -0,0 +1 @@ | |||
v10.0.0 |
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 the v
should be omitted but will probably work as is. Also I'll try to find a way not to force an exact version (i.e. if I already have 10.15
installed, ideally nvm use
would just use that)
Not blocking now.
ETA: found it. You can just say 10
and it will match any 10.x version you already have installed (it will pick the latest)
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.
executing node -v > .nvmrc
, they added the v
automatically, maybe does not need it but best practices. no sure...
Resolved #2
🛠 Purpose
Setting up different static testing tools.
Version requires to make the previous tools to work
nvm
file has been addedHusky hooks
Now every time a dev want to commit something the following will occur:
in case 4 fails (prettier) there is a script name
prettier:format
that can fix that automatically and let the dev add the changes to their commitNEXT STEP
Setup semaphore 2 and make this run also on the CI.