Skip to content
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

Addon-jest: UI Redesign #7424

Merged
merged 47 commits into from
Jul 20, 2019
Merged

Addon-jest: UI Redesign #7424

merged 47 commits into from
Jul 20, 2019

Conversation

CodeByAlex
Copy link
Member

@CodeByAlex CodeByAlex commented Jul 16, 2019

What I did

Redesigned and refactored the Jest addon to follow the pattern of the accessibility addon.
Parsing now works with new jest output format as well as the old format.
Continuation of #7395

There is more data coming to this addon but this is just a redesign.
jest-design-demo

Desktop, tablet, and wide phones [UPDATED]:
image

Mobile [UPDATED]:
image

How to test

  • Is this testable with Jest or Chromatic screenshots? yes
  • Does this need a new example in the kitchen sink apps? no
  • Does this need an update to the documentation? no

If your answer is yes to any of these, please make sure to include it in your PR.

CodeByAlex and others added 30 commits June 18, 2019 05:51
…similar to the accessibility addon for a more cohesive experience
…o add color to the expected and received values
Removed console log
…s (ex. no longer checking for expected/result/etc.). Made it backwards compatible with previous result format.
…(one included breaks between title and value and the other outputs both in one line)
…eature/jest-redesign

# Conflicts:
#	package.json
#	yarn.lock
@domyen
Copy link
Member

domyen commented Jul 17, 2019

👏 This looks amazing @CodeByAlex!

Things I noticed from a visual design POV:

  • Remove the extra horizontal borders (It looks like there are two between the subheader and first item)
  • Reduce the dimensions of the status gauge to 30x6 (i.e., make it shorter)
  • Do we need the % passed? I can see how this is useful in aggregate across a whole app, but it doesn't add much information at the component level. I'm likely only encountering a handful of tests at the component level.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome improvement -- this is so much better! 💯

Left a couple code changes. The unit tests are for documentation more than anything in this case, I'm assuming they work.

addons/jest/src/components/Result.tsx Outdated Show resolved Hide resolved
return [msg];
};

const getConvertedText: (msg: string) => MsgElement[] = (msg: string) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a couple unit tests for this?

return elementArray;
};

const getTestDetail: (msg: string) => TestDetail = (msg: string) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a couple unit tests for this?

addons/jest/src/components/Message.tsx Outdated Show resolved Hide resolved
addons/jest/src/components/Message.tsx Outdated Show resolved Hide resolved
addons/jest/src/components/Panel.tsx Outdated Show resolved Hide resolved
addons/jest/src/components/Panel.tsx Outdated Show resolved Hide resolved
addons/jest/src/components/Panel.tsx Outdated Show resolved Hide resolved
addons/jest/src/components/Panel.tsx Outdated Show resolved Hide resolved
addons/jest/src/components/Panel.tsx Outdated Show resolved Hide resolved
) : null}
</SuiteHead>
<TabsState initial="failing-tests" backgroundColor="rgba(0,0,0,.05)">
<div id="failing-tests" title={`${failedNumber} Failed`} color="#FF4400">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use the theme colors rather than being hardcoded in.

})}
</List>
</div>
<div id="passing-tests" title={`${successNumber} Passed`} color="#66BF3C">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theme colors 🙏

<Icon
icon="chevrondown"
size={10}
color="#9DA5AB"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

theme.color.mediumdark

@CodeByAlex
Copy link
Member Author

Thanks, @dom. I'll get those colors turned over to tokens asap. And it does look cleaner with your suggestions so I'll commit those changes.

Cheers, @shilman. Ill turn the component into a hook so you don't 🤢 and add some unit tests

{tests ? (
<ContentWithTheme tests={tests} />
) : (
<NoTests>This story has no tests configured</NoTests>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to add some documentation here, on how to get it set up so there ARE tests.

icon="chevrondown"
size={10}
color="#9DA5AB"
style={{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be in a rotate prop.

@ndelangen
Copy link
Member

Code looks good as is, some improvements would be good, but I'd be ok merging as is, and improving it later.

@ndelangen ndelangen self-assigned this Jul 19, 2019
@CodeByAlex
Copy link
Member Author

CodeByAlex commented Jul 19, 2019

Code looks good as is, some improvements would be good, but I'd be ok merging as is, and improving it later.

@ndelangen Sounds good. I can definitely add more tests, docs, and tweaks in the next PR based on these comments if that works for everyone. Would like to get feedback as soon as possible from users.

@shilman shilman changed the title Feature/jest redesign Addon-jest: UI Redesign Jul 20, 2019
@shilman shilman merged commit 81e3996 into next Jul 20, 2019
@shilman shilman deleted the feature/jest-redesign branch July 20, 2019 03:42
@fabiradi
Copy link
Contributor

Unfortunately, the redesign handles skipped tests (pending) and todos (todo) differently: They currently don't show up in the list. Previously, they were counted as "failed", but shown in "green" (like "passed"). This was still not perfect, but – at least – they were displayed.

jest-failed-tests

test.todo('This is a todo');
xit('This is a skipped test', () => {});

ℹ️ jest test lists skipped tests and todo separately in its summary.
jest-summary

It would be nice to have all tests listed, either as passed or failed. Displaying skipped tests as todos differently is something for a feature report ...

@ndelangen
Copy link
Member

Should be easy to add a few extra tabs for those types of tests reports I think.

Would you be interested in contributing that feature @fabiradi ?

@fabiradi
Copy link
Contributor

Should be easy to add a few extra tabs for those types of tests reports I think.

Would you be interested in contributing that feature @fabiradi ?

😃 I actually thought about it and already took a peek into the source code. The counts are "simple" filters on strings and should be easy to duplicate for a couple of more tabs – fixing the wrong "failed" count.

const successNumber = result.assertionResults
    .filter(({ status }) => status === 'passed').length;
const failedNumber = result.assertionResults.length - successNumber;

I have three questions up front:

  1. Colors: Where can I find suitable color definitions? The Storybook brand colors are not used for red (failed) and green (passed) tests. How would a get a yellow and violet? I could use "Gold" and "Purple" from the brand guide.
  2. Visiblity of Tabs: Would the tabs for "skipped" and "todo" be displayed all the time or hidden when the count = 0? The failed tab seems to be visible even when no test failed. I'm not sure if that would be okay for skipped and todo ...
  3. Form of Contribution: Where/how would I contribute? Is this a separate issue or an addition to this issue?

👋 As this would be my first contribution, I still have to take a look into the contribution guide...

@CodeByAlex
Copy link
Member Author

@fabiradi That sounds great!

You can check out the brand colors here: https://storybook-design-system.netlify.com/?path=/docs/design-system-color--all
I think the tabs should be persistent in the same way that the a11y addon tabs are.
The form of contribution is up to you. Maybe create a feature request and then reference it in the PR.

Looking forward to seeing this addition 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants