Calcite-components: Docs: add RTL stories for Storybook docs/Screener tests
Summary
Now that #1831 Epic has landed, ~we need e2e tests which confirm the component can inherit dir="rtl" from further up the DOM tree.~
Update per this comment:
I'll remove the rtl tests since they're just testing the browser default and we have the new ESLint rule, which checks for dir attribute on the host.
~(I'm thinking the tests can use .getComputedStyle() to check that the component's dir CSS property value is rtl.)~
Measure of Success
~All components have an e2e test which asserts that the component can inherit its rtl direction from an ancestor.~
All components have an RTL story for Screener test/Storybook docs.
Resources
This is a follow-on that we're hoping will help us verify all changes from Epic #1831.
caripizza
All 13 comments
@jcfranco can you take this one? I'm not sure how it will work because testing 'dir' would pass if the 'dir' tag is on it.
driskull
on 6 May 2021
jcfranco
on 6 May 2021
What about something like this?
describe("when its text direction is RTL", () => {
it("can inherit text direction from further up the DOM tree", async () => {
const page = await newE2EPage({
html: `
<html lang="he" dir="rtl">
<body>
<calcite-tile
heading="זה הכותרת של האריח"
></calcite-tile>
</body>
</html>
`
});
const tileEl = await page.find("calcite-tile");
expect(await (await tileEl.getComputedStyle()).getPropertyValue("direction")).toEqual("rtl");
});
it("can receive its own text direction", async () => {
const page = await newE2EPage({
html: `
<section dir="ltr">
<calcite-tile
dir="rtl"
heading="זה הכותרת של האריח"
></calcite-tile>
</section>
`
});
const tileEl = await page.find("calcite-tile");
expect(await (await tileEl.getComputedStyle()).getPropertyValue("direction")).toEqual("rtl");
});
});
Yeah that should work. 👍
It should also make sure that the dir attribute is not applied onto the component tag in the first scenario.
driskull
on 12 May 2021
Do we need to test for something internally? With the new @calcite-components/ban-props-on-host ESLint rule, dir should no longer be allowed on the host, so the above snippet is testing default browser behavior and nothing from the components themselves.
jcfranco
on 19 May 2021
@jcfranco should I rework the commonTests then? (I only added them to tile, tile-select, and tile-select-group to start)
https://github.com/Esri/calcite-components/blob/master/src/components/calcite-tile-select/calcite-tile-select.e2e.ts#L132
caripizza
on 19 May 2021
Not unless we can find a way to generalize the screenshot diffing you've got there. Maybe we can rely on Screener for this and have each component have an RTL story?
jcfranco
on 19 May 2021
Works for me! I almost created the issue yesterday but ran out of time. Here's the notes I took:
[x] Based on a quick audit of master, these components that were touched in 1831 don't have RTL stories yet and should:
- Button
- Card
- Checkbox
- Date Picker
- Input (story should include Input Message too)
- Input Date Picker
- Label
- Link
- Modal
- Pagination (only has a knob option right now)
- Radio Button
- Radio Button Group
- Radio Group
- Select (only has a knob option right now)
- Tabs
(Left out Avatar, since nothing in UI would need to change in RTL.)
[x] These components have issues with their RTL stories:
- Icon - its RTL story needs
flip-rtlset with an icon likearrowLeftor similar (ie., show the UI difference in rtl) - Rating - its RTL story needs some stars checked or similar (ie., show the UI difference in rtl)
- Icon - its RTL story needs
[x] These components weren't touched in 1831, but need RTL stories and/or story improvements:
- Accordion (add story that shows it in rtl by default)
- Action (add story that shows it in rtl by default, with
textEnabledset to true) - Action Bar (add story that shows it in rtl by default)
- Action Pad (add story that shows it in rtl by default)
- Block (add story that shows it in rtl by default)
- Color Picker (add story that shows it in rtl by default)
- Inline Editable (add story that shows it in rtl by default)
- Flow (add story that shows it in rtl by default)
- Shell (add story that shows it in rtl by default)
- ~Tile~
- ~Tile Select~
- ~Tile Select Group~
- Tip Manager (add story that shows it in rtl by default)
- Tip (add story that shows it in rtl by default)
- Tooltip
- Tree
- Value List (add story that shows it in rtl by default)
(Left out Slider, since it doesn't yet support RTL - issue #720.)
caripizza
on 19 May 2021
I'll remove the rtl tests since they're just testing the browser default and we have the new ESLint rule, which checks for dir attribute on the host.
I'll also remove the experimental screenshot tests from the test script. (We'll rely on Screener stories for the screenshot testing moving forward, since we get the added benefit of their detailed regression information like the way style changes are recorded.)
caripizza
on 21 May 2021
Based on a quick audit of master, these components that were touched in 1831 don't have RTL stories yet and should:
☝🏼 will be addressed by #2228
jcfranco
on 28 May 2021
Above PR is installed. 🎉
jcfranco
on 1 Jun 2021
After checking latest beta.56 docs, all components now have RTL stories in our Storybook, except:
- _Avatar, Loader (no visual different in RTL, so they don't need one)_
- Progress, Slider (AFAIK, these don't have RTL support yet, so they need support first then we can add stories)
- Input Time Picker, Scrim (these need RTL stories)
caripizza
on 8 Jun 2021
Closing as verified. @caripizza Can you create a follow-up issue for ☝🏼 ? 🙇🏼♂️
jcfranco
on 8 Jun 2021
Related issues
nwhittaker
·
5Comments
joeyHarig
·
4Comments
driskull
·
4Comments
driskull
·
3Comments
jcfranco
·
3Comments