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

[OffcanvasHeader.js] Update API for HTML standard, simplify attribute naming, & migrate to named slots #216

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

sreidthomas
Copy link
Contributor

WHAT WAS DONE:

Update component API to adhere to HTML standard microsyntaxes, simplify attribute naming conventions, and migrate to named slots with basic markup where possible

ISSUE:

We have some components that handle attribute values in non-standard ways, use non-standard attribute naming conventions, and unnecessarily nest custom components instead of using named slots.

OVERALL TASK / JOB DUTIES:

  1. Update component APIs to adhere to all HTML standard microsyntaxes, including boolean attributes, colors, space-separated tokens, and comma-separated tokens.
  2. Simplify attribute naming conventions by removing data-* prefixes and using existing standard or global attribute names where possible.
  3. Migrate to named slots with basic markup where possible, removing the nesting of custom web components and the use of content passed via attributes in favor of named slots.

ADDITIONAL NOTES:

Nested, custom components should only be used when:

  1. The nested custom component is a standalone component itself. E.g., it's OK to use in a custom component slot.
  2. The nested content requires more than one level of depth in the DOM. Slots cannot support this case.

Each time we update component attributes or migrate to named slots, we'll need to:

• Update the attribute in the component JS implementation.
• Update the attribute definition and Storybook param names in the story file.

Components are distributed in this spreadsheet:

https://is.gd/ETzUCF

Breakdown of individual tasks for each issue:

Task 201: Update component API to adhere to HTML standard microsyntaxes

  1. Identify components with attributes that do not conform to HTML standard microsyntaxes.
  2. Update attribute values to adhere to standard microsyntaxes:
    • For boolean attributes, ensure values are true or false instead of non-standard values.
    • For color attributes, ensure values are valid color representations (e.g., hex, RGB, or color names).
    • For attributes with space-separated tokens, ensure tokens are separated by spaces.
    • For attributes with comma-separated tokens, ensure tokens are separated by commas.

Task 202: Simplify attribute naming conventions

  1. Identify components with attributes that use non-standard naming conventions or unnecessary prefixes (e.g., data-*).
  2. Standardize attribute names by:
    • Removing data-* prefixes for attributes that behave like global or standard element attributes.
    • Using existing global or standard element attribute names where possible.
    • Removing unnecessary prefixes for custom attributes.
    • Ensuring consistency in attribute names across components.
  3. For each updated attribute or migration to named slots:
    • Update the attribute in the component JS implementation.
    • Update the attribute definition and Storybook param names in the story file.

Task 205: Migrate to named slots with basic markup where possible

  1. Identify components that accept content via attributes or use nested custom components unnecessarily.
  2. Migrate to named slots by:
    • Removing nested custom components and using named slots instead.
    • Refactoring components to accept content via named slots instead of attributes.
    • Updating component documentation and examples to reflect the use of named slots.

@sreidthomas
Copy link
Contributor Author

sreidthomas commented May 16, 2024

COMPLETED TASK FOR MAY 16, 2024:

Task 202: Simplify attribute naming conventions

  • Remove data- Prefix for Global or Standard Attributes:

There are no attributes in the OffcanvasHeader component that behave like global or standard element attributes, so no changes are needed here.

  • Remove data- Prefix for Other Attributes:

In the OffcanvasHeader component, we have the following attributes with the data- prefix:

data-parent-id
data-button-dark
data-extra-classes
data-expand

After the update, these attributes should be:

parent-id
button-dark
extra-classes
expand

These values were also updated:
this.closeBtn.setAttribute('img-alt', '');
this.closeBtn.setAttribute('icon', '');
this.closeBtn.setAttribute('close', 'true');
this.closeBtn.setAttribute('bs-dismiss', parentID);

  • Ensure Consistent Attribute Names and Values:

The OffcanvasHeader component does not have attributes that are used in multiple components, so this step does not apply.

  • Update Attribute Names for Similar Functionality:

There are no attributes in the OffcanvasHeader component that have similar functionality to attributes in other components, so no changes are needed here.

  • Update Attribute Values for Consistency:

The attributes in the OffcanvasHeader component (data-parent-id, data-button-dark, data-extra-classes, data-expand) are specific to its functionality and do not need to be updated for consistency with other components.

@sreidthomas
Copy link
Contributor Author

sreidthomas commented May 17, 2024

COMPLETED TASK FOR MAY 17, 2024:

Task 201: Update component API to adhere to HTML standard microsyntaxes

  • Boolean Attributes: close should be a boolean attribute & since we need to adhere to the standard microsyntax, we leave it as a string here ('true').

  • Color Attributes: None of the attributes in this component are color-related.

  • Space-Separated Tokens: No attribute is separated by space. Most attributes are using -

  • Comma-Separated Tokens: No attributes in this component use comma-separated tokens.

NOTE:

Added data- back to the lines below until I work on the cod-button component:
this.closeBtn.setAttribute('img-alt', '');
this.closeBtn.setAttribute('icon', '');
this.closeBtn.setAttribute('close', 'true');
this.closeBtn.setAttribute('bs-dismiss', parentID);
this.closeBtn.setAttribute('data-extra-classes', 'btn-close-white')

@sreidthomas
Copy link
Contributor Author

sreidthomas commented May 20, 2024

May 20, 2024 Update:

Opened up branch: issue.201-2-5.2 for the Button.js component in order to update attributes there so these lines can be properly updated in OffcanvasHeader.js:

this.closeBtn.setAttribute('img-alt', '');
this.closeBtn.setAttribute('icon', '');
this.closeBtn.setAttribute('close', 'true');
this.closeBtn.setAttribute('bs-dismiss', parentID);

@sreidthomas
Copy link
Contributor Author

sreidthomas commented May 21, 2024

May 21, 2024 Update:

Updated the following attributes:

data-parent-id
data-button-dark
data-extra-classes
data-expand (Not sure about this one)

After the update, these attributes should be:

parent-id (updated)
button-dark (updated)
extra-classes (updated)
expand (Not sure about this one)

this.closeBtn.setAttribute('data-img-alt', '');
this.closeBtn.setAttribute('data-icon', '');
this.closeBtn.setAttribute('data-close', 'true');
this.closeBtn.setAttribute('data-bs-dismiss', parentID);

@sreidthomas
Copy link
Contributor Author

sreidthomas commented May 21, 2024

QUESTIONS FOR OFFCANVASHEADER.JS: @maxatdetroit

  1. Does this attribute need to be update? It's not listed with the other attributes. It's only inside the EventListener:
_onClick() {
    this.getRootNode()
      .host.getRootNode()
      .host.setAttribute('data-show', 'false');
  }
  1. There is only one offcanvas.stories.js file but there is an OffcanvasHeader.js and OffcanvasBody.js do they all go together?
  2. If its setting or getting an attribute those lines should be update. For example, should this attribute be updated -->
    const expand = this.getAttribute('data-expand');

@sreidthomas
Copy link
Contributor Author

NEXT STEP AS OF MAY 21, 2024:

Task 205: Migrate to named slots with basic markup where possible

  1. Identify components that accept content via attributes or use nested custom components unnecessarily.
  2. Migrate to named slots by:
    • Removing nested custom components and using named slots instead.
    • Refactoring components to accept content via named slots instead of attributes.
    • Updating component documentation and examples to reflect the use of named slots.

@sreidthomas
Copy link
Contributor Author

NOTE TO SELF:

yarn lint & yarn prettier -w "FILENAME" ==> how to fix prettier issue

@maxatdetroit
Copy link
Member

QUESTIONS FOR OFFCANVASHEADER.JS: @maxatdetroit

1. Does this attribute need to be update? It's not listed with the other attributes. It's only inside the EventListener:
_onClick() {
    this.getRootNode()
      .host.getRootNode()
      .host.setAttribute('data-show', 'false');
  }

Yes, it needs to be updated. This is where it gets set:

this.getAttribute('data-show') == 'true'
? node.setAttribute('data-show', true)
: 0;

2. There is only one offcanvas.stories.js file but there is an OffcanvasHeader.js and OffcanvasBody.js do they all go together?

Yes, they do. See the story file or storybook page for offcanvas to see how they get used together.

3. If its setting or getting an attribute those lines should be update. For example, should this attribute be updated -->
   ` const expand = this.getAttribute('data-expand');`

Thanks for the example. Yes, both 'setters' and 'getters' should be updated if we update the attribute name.

@sreidthomas
Copy link
Contributor Author

When updating to named slots, should we also change how the styles are being added @maxatdetroit?

Something this:

namedslots

@maxatdetroit
Copy link
Member

@sreidthomas no, you can continue to add the styles like they were previously (using javascript in the constructor).

We're going to rework how we add stylesheets as part of #171.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants