Generalize mocking of states in dummy pages #314

didoo · 2022-05-25T09:22:30Z

📌 Summary

This task has been on my todo list for a long time, and while implementing the states for the form controls elements I have realized it was time to bite the bullet and implement it properly (without the proposed changes, would have required a lot of hacks).

What I am proposing here is to replace the way we declare and handle the visual states for the showcase of components, from using Ember arguments (@state) passed down in the actual components code (HBS + SCSS), to custom HTML attributes (mock-state) applied only in the dummy webpages (and referenced only in the CSS).

What is the problem I am trying to solve?

The fact is that with the previous approach we were mixing two concerns, the code for the actual components, and the code used to showcase them in the dummy website, and we were exposing both in production code.

Why this solution is better?

With the proposed change, we get rid of this entanglement on the HBS template side (and we may be able to get rid of the same thing on the CSS side, in the future, using a post-css processor that removed the mock-[state] selectors from the generated CSS for production code).

🛠️ Detailed description

In this PR I have

added a controller for the “components” pages that takes care of handling mock-state attributes
- the logic is this: we have two possible custom HTML attributes (not prefixed with data intentionally, even if it's not valid HTML for our purpose is OK)
  - mock-state-value is the value of the state that we want to use, to apply a class mock-[state] to an element, so that a custom CSS class can be used to emulate a particular state in a component (typically pseudo states like :hover or :focus) in a showcase
  - mock-state-selector is the DOM selectors we want to use (relative to the element to which is applied) to select the target element on which dynamically apply the mock class at runtime; if no selector is defined, we simply use the element with the mock-state-value attribute itself
- at the moment I don't see this functionality used in other type of pages, so it's OK to have it only here (if other pages/sections will need the same functionality, we will abstract/duplicate it accordingly)
updated Dropdown + Button dummy pages (and related components and CSS files) to use the new mock-status approach

Notice: since the other pages using the states showcase are Link::CTA and Link::Standalone, we will apply the changes to the remaining dummy pages once this branch is merged to mainand main` merged in #217 (we will apply the changes there).

🖤 Credits

Thanks to @jgwhite 🙏 for helping me solve the problem with extends Component + constructor() + scheduleOnce('afterRender'...). Essentially this is the ultimate gotcha with controllers — they’re singletons. So that constructor will only be run once for the lifetime of the application.. TIL 🤷‍♂️

(the code fix is in ddfc49a)

👀 How to review

👉 Review by files changed

Notice: I'll add comments inline in the files changed section, to explain what I did and why.

Reviewer's checklist:

+1 Percy if applicable

💬 Please consider using conventional comments when reviewing this PR.

…ng `mock-state` attributes

… CSS files) to use the new `mock-status` approach

changeset-bot · 2022-05-25T09:22:33Z

⚠️ No Changeset found

Latest commit: a3ad600

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel · 2022-05-25T09:22:33Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
hds-components	✅ Ready (Inspect)	Visit Preview	May 25, 2022 at 2:20PM (UTC)
hds-flight-website	✅ Ready (Inspect)	Visit Preview	May 25, 2022 at 2:20PM (UTC)

didoo · 2022-05-25T09:23:25Z

packages/components/app/styles/components/button.scss

@@ -29,13 +29,13 @@ $hds-button-focus-border-width: 3px;
  // This covers all of the browsers and focus scenarios (due to the custom focus design).
  &:disabled,
  &[disabled],
-  &.is-disabled,
+  &.mock-disabled,


in the future, this class selectors could be removed using a css post-processor

didoo · 2022-05-25T09:24:41Z

packages/components/app/styles/mixins/_focus-ring.scss

@@ -8,7 +8,8 @@

  // default focus for browsers that still rely on ":focus"
  &:focus,
-  &.is-focus {
+  &.is-focus,


once the changes done across all the pages, I'll remove the is-[state] selectors from here, for now we need both to avoid breaking the percy tests

didoo · 2022-05-25T09:30:23Z

packages/components/tests/dummy/app/controllers/components.js

+    if (element.attributes['mock-state-selector']) {
+      target = element.querySelector(
+        element.attributes['mock-state-selector'].value
+      );
+    } else {
+      target = element;
+    }
+    const state = element.attributes['mock-state-value'].value;
+    target.classList.add(`mock-${state}`);
+  });


The logic here is this: we have two possible custom HTML attributes (not prefixed with data intentionally, even if it's not valid HTML for our purpose is OK)

mock-state-value is the value of the state that we want to use, to apply a class mock-[state] to an element, so that a custom CSS class can be used to emulate a particular state in a component (typically pseudo states like :hover or :focus) in a showcase

mock-state-selector is the DOM selectors we want to use (relative to the element to which is applied) to select the target element on which dynamically apply the mock class at runtime; if no selector is defined, we simply use the element with the mock-state-value attribute itself

didoo · 2022-05-25T09:31:28Z

packages/components/tests/dummy/app/templates/components/button.hbs

+              @text={{capitalize state}}
+              @size={{size}}
+              @color={{color}}
+              mock-state-value={{state}}


here you can see how one of these mock states is declared on a component, but only in the dummy documentation page

didoo · 2022-05-25T09:32:48Z

packages/components/tests/dummy/app/templates/components/dropdown.hbs

+        <Hds::Dropdown::ListItem::CopyItem
+          @text="{{state}}: 91ee1e8ef65b337f0e70d793f456c71d"
+          mock-state-value={{state}}
+          mock-state-selector="button"


here instead you can see a case where the target element is inside the component, but using the mock-state-selector it's possible to apply the mock state class directly to it, without the need to pass down arguments across components and sub-components

…multiple targets

Ah yeah, this is the ultimate gotcha with controllers — they’re singletons. So that constructor will only be run once for the lifetime of the application.

didoo · 2022-05-25T14:14:09Z

@alex-ju @MelSumner @Dhaulagiri now the PR is ready for review.

alex-ju

I very much prefer using attributes to mock states, especially as I got a bit confused at my first interaction with the repo when looking at those classes. Code looks good to me (as much as I understand the lifecycle of an Ember instance at the moment).

Dhaulagiri · 2022-05-25T18:53:07Z

packages/components/tests/dummy/app/controllers/components.js

+export default class ComponentsController extends Controller {
+  @service router;
+
+  constructor() {


this all feels a bit magical to me but I also don't have a more idiomatic suggestion springing to mind. I think this is ok for "scrappy" but something we should at least be remembering if we run into unforeseen issues in the future.

MelSumner

I am surprised that Ember is not throwing several errors on this, especially re: having a controller named "components".
I am curious why you didn't use a data- attribute; seems strange to purposefully craft invalid HTML when valid HTML is available for exactly this sort of thing. 🤔
The approach itself seems similar to what we do with ember-a11y-refocus

didoo · 2022-05-26T14:12:51Z

I am surprised that Ember is not throwing several errors on this, especially re: having a controller named "components".

Me too :)

I am curious why you didn't use a data- attribute; seems strange to purposefully craft invalid HTML when valid HTML is available for exactly this sort of thing. 🤔

Shorter, easier to read but more importantly to spot in code or in devtools (there may be other legit data- attributes).

The approach itself seems similar to what we do with ember-a11y-refocus

👍

didoo added 2 commits May 25, 2022 09:55

added controller for the “components” pages that takes care of handli…

6cef50b

…ng `mock-state` attributes

updated Dropdown + Button dummy pages (and related components and…

479aea4

… CSS files) to use the new `mock-status` approach

didoo requested review from alex-ju, Dhaulagiri and MelSumner May 25, 2022 09:22

didoo commented May 25, 2022

View reviewed changes

linting

5420f94

vercel bot temporarily deployed to Preview – hds-flight-website May 25, 2022 09:36 Inactive

vercel bot temporarily deployed to Preview – hds-components May 25, 2022 09:36 Inactive

didoo marked this pull request as draft May 25, 2022 09:46

generalized replaceMockStates function to handle multiple states + …

2ed672e

…multiple targets

vercel bot temporarily deployed to Preview – hds-flight-website May 25, 2022 11:25 Inactive

vercel bot temporarily deployed to Preview – hds-components May 25, 2022 11:25 Inactive

changes suggested by Jamie White to fix issue with constructor()

ddfc49a

Ah yeah, this is the ultimate gotcha with controllers — they’re singletons. So that constructor will only be run once for the lifetime of the application.

vercel bot temporarily deployed to Preview – hds-flight-website May 25, 2022 13:56 Inactive

vercel bot temporarily deployed to Preview – hds-components May 25, 2022 13:56 Inactive

didoo marked this pull request as ready for review May 25, 2022 14:13

updated tests for CTA to avoid Percy failing tests

a3ad600

vercel bot temporarily deployed to Preview – hds-flight-website May 25, 2022 14:19 Inactive

vercel bot temporarily deployed to Preview – hds-components May 25, 2022 14:20 Inactive

alex-ju approved these changes May 25, 2022

View reviewed changes

Dhaulagiri reviewed May 25, 2022

View reviewed changes

MelSumner approved these changes May 26, 2022

View reviewed changes

didoo merged commit 8ca4553 into main May 26, 2022

didoo deleted the generalize-mocking-of-states branch May 26, 2022 14:13

didoo mentioned this pull request May 26, 2022

Mock status - Complete migration #321

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize mocking of states in dummy pages #314

Generalize mocking of states in dummy pages #314

didoo commented May 25, 2022 •

edited

Loading

changeset-bot bot commented May 25, 2022 •

edited

Loading

vercel bot commented May 25, 2022 •

edited

Loading

didoo May 25, 2022 •

edited

Loading

didoo May 25, 2022

didoo May 25, 2022

didoo May 25, 2022

didoo May 25, 2022

didoo commented May 25, 2022

alex-ju left a comment

Dhaulagiri May 25, 2022

MelSumner left a comment

didoo commented May 26, 2022 •

edited

Loading

Generalize mocking of states in dummy pages #314

Generalize mocking of states in dummy pages #314

Conversation

didoo commented May 25, 2022 • edited Loading

📌 Summary

🛠️ Detailed description

🖤 Credits

👀 How to review

changeset-bot bot commented May 25, 2022 • edited Loading

⚠️ No Changeset found

vercel bot commented May 25, 2022 • edited Loading

didoo May 25, 2022 • edited Loading

Choose a reason for hiding this comment

didoo May 25, 2022

Choose a reason for hiding this comment

didoo May 25, 2022

Choose a reason for hiding this comment

didoo May 25, 2022

Choose a reason for hiding this comment

didoo May 25, 2022

Choose a reason for hiding this comment

didoo commented May 25, 2022

alex-ju left a comment

Choose a reason for hiding this comment

Dhaulagiri May 25, 2022

Choose a reason for hiding this comment

MelSumner left a comment

Choose a reason for hiding this comment

didoo commented May 26, 2022 • edited Loading

didoo commented May 25, 2022 •

edited

Loading

changeset-bot bot commented May 25, 2022 •

edited

Loading

vercel bot commented May 25, 2022 •

edited

Loading

didoo May 25, 2022 •

edited

Loading

didoo commented May 26, 2022 •

edited

Loading