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

Simplify attribute naming conventions #202

Open
maxatdetroit opened this issue Apr 19, 2024 · 6 comments
Open

Simplify attribute naming conventions #202

maxatdetroit opened this issue Apr 19, 2024 · 6 comments
Assignees
Labels

Comments

@maxatdetroit
Copy link
Member

maxatdetroit commented Apr 19, 2024

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
Copy link
Member Author

Double check nested duplicate ID

@maxatdetroit
Copy link
Member Author

@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.

@maxatdetroit maxatdetroit self-assigned this Apr 29, 2024
@sreidthomas
Copy link
Contributor

@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
Copy link
Member Author

No stress, take your time :)

@sreidthomas
Copy link
Contributor

sreidthomas commented May 16, 2024

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
Copy link
Member Author

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.

This was referenced May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants