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

🎛️ Create a Stimulus controller w-init to support initial classes #11071

Closed
lb- opened this issue Oct 17, 2023 · 5 comments · Fixed by #11120
Closed

🎛️ Create a Stimulus controller w-init to support initial classes #11071

lb- opened this issue Oct 17, 2023 · 5 comments · Fixed by #11120

Comments

@lb-
Copy link
Member

lb- commented Oct 17, 2023

Is your proposal related to a problem?

We have a small bit of JS that adds the class 'ready' to the body element when the DOM has loaded, this is useful for a few things such as styling elements as hidden until we know the JS is ready.

This pattern is used in a few places, such as the side panel JS where we want the panels to be hidden until we know the logic of the JS is loaded.

I propose a simple Stimulus solution to this that's similar to x-init and x-cloak from Alpine.js - where we use a controller to trigger this behaviour.

Describe the solution you'd like

  1. Create a new controller InitController that adds initial classes and also removes some classes based on data attributes when the JS is ready.
  2. Use this new controller in wagtail/admin/templates/wagtailadmin/skeleton.html
  3. Remove our jQuery approach to this in client/src/entrypoints/admin/core.js
  4. The controller will be pretty simple but can be enhanced later for other usages such as the side panel when that work is being done.
  5. Testing is a must, remember that Jest await jest.runAllTimersAsync(); is your friend, see client/src/controllers/CloneController.test.js for some examples of tests that use this.
  6. nice to have - Remove the initial-class code from the RevealController - this is actually not required and better served with this generic approach when it comes time.

About timing & usage

Stimulus will connect controllers progressively based on the order of the data-controller and all controllers connect async, but as a microtask. This is slightly before a setTimeout would fire (it's a bit nuanced).

This gives us the ability to order our controllers in a way that the w-init will be last and hence will do its classes changes last.

<body  data-controller="w-other w-init" data-w-init-ready-class="ready">

In the above code, w-other will connect first, w-init second.

Rough implementation

Here's a proposed implementation, it has ready and remove classes and the ability to configure the delay, default will be no delay except for the microtask / Promise resolving.

import { Controller } from '@hotwired/stimulus';
import { debounce } from '../utils/debounce';

/**
 * Adds the ability for a controlled element to add or remove classes
 * when ready to be interacted with.
 *
 * @example
 * <div class="hide-me" data-controller="w-init" data-w-init-remove-class="hide-me" data-w-init-ready-class="loaded">
 *   When the DOM is ready, this div will have the class 'loaded' added and 'hide-me' removed.
 * </div>
 */
export class InitController extends Controller<HTMLElement> {
  static classes = ['ready', 'remove'];

  static values = {
    delay: { default: -1, type: Number },
  };

  declare readonly readyClasses: string[];
  declare readonly removeClasses: string[];

  declare delayValue: number;

  /**
   * Allow for a microtask delay to allow for other controllers to connect,
   * then trigger the ready method.
   */
  connect() {
    Promise.resolve().then(() => {
      this.ready();
    });
  }

  ready() {
    // if below zero (default) resolve immediately instead of using a setTimeout
    const delayValue = this.delayValue < 0 ? null : this.delayValue;

    // remember to call the result of `debounce` immediately, it returns a promise that can be used to dispatch a helpful event & allow this controller to remove itself
    debounce(() => {
      this.element.classList.add(...this.readyClasses);
      this.element.classList.remove(...this.removeClasses);
    }, delayValue)().then(() => {
      this.dispatch('ready', { cancelable: false });
      this.remove();
    });
  }

  // To avoid this controller hanging around when not needed, allow it to remove itself when done
  remove() {
    const element = this.element;
    const controllerAttribute = this.application.schema.controllerAttribute;
    const controllers =
      element.getAttribute(controllerAttribute)?.split(' ') ?? [];
    const newControllers = controllers
      .filter((identifier) => identifier !== this.identifier)
      .join(' ');
    if (newControllers) {
      element.setAttribute(controllerAttribute, newControllers);
    } else {
      element.removeAttribute(controllerAttribute);
    }
  }
}

Describe alternatives you've considered

  • We keep this logic ad-hoc in various bits of JS and avoid a generic solution.

Additional context

@lb-
Copy link
Member Author

lb- commented Oct 17, 2023

Flagging as a good first issue for someone keen to write their first Stimulus controller, the scope is small but it will need unit tests and the code snippet above has not been manually validated so may not work as is. Some experience with TypeScript and a willingness to read the Stimulus documentation will be required though.

This new controller will need to be imported and registered in client/src/controllers/index.ts.

Finally, it's recommended that anyone doing this read about Stimulus order & timing here - https://stimulus.hotwired.dev/reference/lifecycle-callbacks#order-and-timing & the linked article about microtasks https://jakearchibald.com/2015/tasks-microtasks-queues-and-schedules/

@Chiemezuo
Copy link
Contributor

Do you recommend I go for it, or leave it for someone who hasn't yet had a first issue and then help out that person instead?

@felp99
Copy link
Contributor

felp99 commented Oct 18, 2023

Hey guys, I'm with a team from my school. We are looking for a good contribution to do in this project. Can you guide us on this issue?

@GetRohitansh
Copy link
Contributor

Do you recommend I go for it, or leave it for someone who hasn't yet had a first issue and then help out that person instead?

Can you do that please, I wanted to get started with open source in python and it would give me good head start.
I am willing to work on this

@Chiemezuo
Copy link
Contributor

Do you recommend I go for it, or leave it for someone who hasn't yet had a first issue and then help out that person instead?

Can you do that please, I wanted to get started with open source in python and it would give me good head start. I am willing to work on this

@GetRohitansh This particular issue is going to require you to know some Javascript and probably Jest testing. Chances are, you might really struggle if you don't have those two requirements covered. If you do, though, then are you on the Wagtail Slack? If so, I could reach out to you there so we don't clutter this comment section.

lb- added a commit to Chiemezuo/wagtail that referenced this issue Oct 24, 2023
lb- added a commit to Chiemezuo/wagtail that referenced this issue Oct 24, 2023
lb- added a commit that referenced this issue Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants