fix(action): fixing RTL display bugs by replacing getElementDir and RTL CSS classes with CSS logical properties

[eriklharper]

Related Issue: #2019

Summary

This PR replaces getElementDir and RTL CSS classes with CSS Logical Properties in calcite-action, which allows Screener to effectively capture the RTL states of this component since a re-render is no longer required when programmatically changing the HTML page direction. This adds 45 new Screener snapshots that capture a full set of visual states using the Screener Steps API and new utilities.

How this works

This PR uses screener-storybook to test for visual changes to storybook stories and introduces the use of the screener storybook Steps to test for interaction changes within calcite-action.

The Steps API allows you to interact or call methods before taking a snapshot of the story.

This will allow us to replace all the RTL and Dark theme stories by calling a method to change to RTL or Dark theme in the same story and take a snapshot.

For example, instead of setting a knob to open a component, we can click the component's button to open it.

This PR:

Uses screener-storybook steps: https://screener.io/v2/docs/test-interactions within storybook to take screenshots after interactions as well as changes for dark theme and RTL.

• Replaces dark and RTL stories for the Popover and Tooltip with Screener steps
• Introduces a storybook helper to setup tests for a story
• Exposes a couple strings of JS scripts for toggling theme and setting knobs on a screener run

The plan

• Do this for all other storybook stories.
• Replace all "Dark" theme and "RTL" stories with screener steps
• Remove any stories where content doesn't change
• Use screener interactions (click/hover) where applicable instead of separate stories or setting knob values.

Proposed plan for testing

• In order to test the changes, you have to look at the screener build.
• You should see new screenshots with the name of the snapshots that are provided in the steps code.

Resources

Screener-runner executeScript types: https://github.com/screener-io/screener-runner/blob/ad615d12691c52ac13d069df960f204725486cfd/src/steps.js

Acceptance Criteria

• Setup a comprehensive set of snapshots for calcite-action
• Create an isolated Screener test of calcite-action before the Screener steps are added
• Create an isolated Screener test of calcite-action with all possible prop permutations. UPDATE: this won't be possible because of a call stack size exceeded exception once the number of chained methods reaches a certain point.
• Compare the two Screener runs above to see how much the time differs.

BEFORE: (15 seconds)

AFTER: (using 45 new snapshots for calcite-action + the rest of the existing storybook snapshots)

[driskull]

The code changes look good @eriklharper.

I wonder if we should change the PR to be a type of refactor instead of test?

Ideally, it could maybe be 3 different PRs

• refactor(action): Use css logical properties.
• test(action): Use screener steps.
• fix(action): Appearance should only be clear or solid.

Does that sound right?

Otherwise, those changes are lost if it just uses "test".

[eriklharper]

The code changes look good @eriklharper.

I wonder if we should change the PR to be a type of refactor instead of test?

Ideally, it could maybe be 3 different PRs

• refactor(action): Use css logical properties.
• test(action): Use screener steps.
• fix(action): Appearance should only be clear or solid.

Does that sound right?

Otherwise, those changes are lost if it just uses "test".

How about to keep things simple, I rename the PR title to fix(action): deprecate rtl classes and remove unsupported outline appearance that way the important part is communicated in the release notes. Is there any use for test() commit labels to anyone else outside of Core?

[driskull]

How about to keep things simple, I rename the PR title to fix(action): deprecate rtl classes and remove unsupported outline appearance that way the important part is communicated in the release notes. Is there any use for test() commit labels to anyone else outside of Core?

Sounds reasonable to me. 👍 Unless anyone else has any objections.

[jcfranco]

Unless the outline appearance changes are related to the RTL class removal, which I believe they are not, they should be split into separate PRs.

[eriklharper]

Unless the outline appearance changes are related to the RTL class removal, which I believe they are not, they should be split into separate PRs.

The outline appearance changes are actually related to the Screener steps usage, since the snapshot for appearance="outline" is invalid because it doesn't produce the intended result for what constitutes an outline around the button like it does in other components.

[driskull]

👍

[jcfranco]

LGTM! I just had a couple of questions:

1. Based on an earlier discussion, we decided that the steps API would be considered for advanced testing scenarios. Does that affect the current state of calcite-action's stories, i.e., should some step scenarios be covered by individual stories?
2. Can you revisit the PR title to express what an end-user gets as a fix?

[eriklharper]

LGTM! I just had a couple of questions:

1. Based on an earlier discussion, we decided that the steps API would be considered for advanced testing scenarios. Does that affect the current state of calcite-action's stories, i.e., should some step scenarios be covered by individual stories?

Probably not necessary since the steps API is so easy to use to produce the kinds of results we want. The only place where I see making more than 1 story necessary is if there's a hurdle with producing the desired visual result outside of what is possible with Storybook and/or the Steps API, or if there's a scenario where we want to visually capture a component when used in combination with another component (like parent-child relationships or inter-dependent visual tests) in addition to snapshotting the component in total isolation.

2. Can you revisit the PR title to express what an end-user gets as a fix?

Done. Reworded it to say it fixes RTL display bugs, which is more relevant to the end user.