CityOfDetroit / COD-Design-System

Design system for the City of Detroit.
https://cityofdetroit.github.io/COD-Design-System/
MIT License
1 stars 3 forks source link

Simplify attribute naming conventions #202

Open maxatdetroit opened 5 months ago

maxatdetroit commented 5 months ago

Is your feature request related to a problem? Please describe.

Many of our components re-declare and rename global attributes or standard element attributes with custom names. For example data-disable is intended to behave like the disabled HTML attribute.

Also, many of our components use the data-* custom attribute naming convention. As discussed below, this is not necessary and superfluous.

Describe the solution you'd like

Go through each of our components, and:

  1. If the attribute behaves like a global attribute or standard element attribute
    1. Remove data- prefixes (e.g. data-id becomes id)
    2. Use existing global or standard element attribute names where possible (e.g. data-disable becomes disabled)
  2. Else if the attribute has a data- prefix
    1. Remove data- prefixes (e.g. data-extra-classes becomes extra-classes)
  3. Else:
    1. Look for other places in the codebase where we use a similar/same attribute and make sure the attribute names and accepted values are consistent. E.g.:
      • cod-button has a size attribute that expects a value of sm, md, lg but cod-icon has a size attribute that expects a value of small, medium, large etc. They should both accept the abbreviated version.
      • cod-action-button has a button-color attribute and cod-button has a data-background-color attribute but they effectively do the same thing. They should both be named color and accept values from bootstrap colors (primary, secondary, etc.)
      • cod-container has a data-background-color attribute that expects color-1, color-2 etc. instead of primary, secondary, etc. It should be updated to use bootstrap colors (primary, secondary, etc.).

Each time we update component attributes, we'll need to:

  1. Update the attribute in the component JS implementation (e.g. CodButton.js)
  2. Update the attribute definition and storybook param names in the story file (e.g. cod-button.stories.js)

Additional Context

It seems we started off using the data-* custom attribute naming convention to avoid overwriting existing global/standard HTMLElement attributes and be compatible with future global/standard HTMLElement attributes and enable easier HTML validation (1). However, this seems to be a non-problem for the most part because:

  1. Autonomous custom elements are not validated in HTML validators and their behavior has to be implemented from scratch anyway (so default HTML / DOM behaviors for global/standard attributes is irrelevant)
  2. We don't have any customized built-in elements in our codebase
maxatdetroit commented 5 months ago

Double check nested duplicate ID

maxatdetroit commented 5 months ago

@sreidthomas , I thought we could break down this issue (and #201, #205) between the two of us depending on the component. I marked off which components I'll work on in this spreadsheet: https://is.gd/ETzUCF. It'll help us avoid merge conflicts if we break down the work this way per component.

sreidthomas commented 5 months ago

@sreidthomas , I thought we could break down this issue (and #201, #205) between the two of us depending on the component. I marked off which components I'll work on in this spreadsheet: https://is.gd/ETzUCF. It'll help us avoid merge conflicts if we break down the work this way per component.

Thank you. You might have a nice head start. I am still trying to get the close button for the alert component positioned right. Just playing around with the solution I have. I may need to change something.

maxatdetroit commented 5 months ago

No stress, take your time :)

sreidthomas commented 4 months ago

I think offcanvasHeader.js is a good one to start with:

  connectedCallback() {
    // Nav attributes
    const parentID = this.getAttribute('data-parent-id');
    const btnDark = this.getAttribute('data-button-dark');
    const extraClasses = this.getAttribute('data-extra-classes');
    const offcanvasHeaderClasses = ['offcanvas-header'];
    this.offcanvasTitle.className = 'offcanvas-title';
    this.offcanvasTitle.id = `${parentID}-label`;
    this.closeBtn.setAttribute('data-img-alt', '');
    this.closeBtn.setAttribute('data-icon', '');
    this.closeBtn.setAttribute('data-close', 'true');
    this.closeBtn.setAttribute('data-bs-dismiss', parentID);
  }

None of these are global attributes or standard element attributes but they are using the data- prefix so I would have to shorten these:

data-parent-id ===> parent-id data-button-dark ===> button-dark data-extra-classes ===> extra-classes

would these have to be shortened too @maxatdetroit ?:

this.closeBtn.setAttribute('data-img-alt', '');
this.closeBtn.setAttribute('data-icon', '');
this.closeBtn.setAttribute('data-close', 'true');
this.closeBtn.setAttribute('data-bs-dismiss', parentID);
maxatdetroit commented 4 months ago

would these have to be shortened too @maxatdetroit ?:

this.closeBtn

this.closeBtn is a separate component (cod-button), so I wouldn't address those attributes until you work on that component (cod-button) in a separate PR.