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

sreidthomas · 2024-05-20T21:13:11Z

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:

Update component APIs to adhere to all HTML standard microsyntaxes, including boolean attributes, colors, space-separated tokens, and comma-separated tokens.
Simplify attribute naming conventions by removing data-* prefixes and using existing standard or global attribute names where possible.
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:

The nested custom component is a standalone component itself. E.g., it's OK to use in a custom component slot.
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

Identify components with attributes that do not conform to HTML standard microsyntaxes.
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

Identify components with attributes that use non-standard naming conventions or unnecessary prefixes (e.g., data-*).
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.
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

Identify components that accept content via attributes or use nested custom components unnecessarily.
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 · 2024-05-20T21:17:16Z

UPDATE AS OF MAY 20, 2024:

This component is used in the OffcanvasHeader.js file so in order to properly update OffcanvasHeader.js so attributes need to be changed here:

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

In Progress:

Task 202: Simplify attribute naming conventions part 1 & 2

Did more research on standard, global and custom attributes. Here is what I have so far:

Task 202: Simplify attribute naming conventions part 3

For all the attributes updated in Button.js, the same attributes and params were updated in the Button.stories.js file per instruction --> 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.

sreidthomas · 2024-05-21T13:33:35Z

QUESTIONS: @maxatdetroit

If the component is not using slots are we still supposed to migrate to named slots?
When you migrate to named slots are we only updating the template and story files? Is there always something specific that needs to be updated with each element besides the template and story files e.g. EventListeners?
When we migrate to named templates do we change how we add styles, varStyles, & bootstrapStyles to the component?
Are we only checking for the tasks with issue 201 in the .js files or the stories.js file too?

sreidthomas · 2024-05-21T14:11:51Z

CURRENT ISSUES AS OF MAY 21, 2024:

DO THIS ONCE YOU GET TO COD-ICON PR

The cod-button uses cod-icon so the attributes in cod-icon need to be updated to properly reflect the changes in cod-button

NEXT COMPONENT TO START ON --> cod-icon

Update the attributes in cod-icon and re-test to make sure cod-button is working correctly with the icons

sreidthomas · 2024-05-21T14:18:56Z

NEW TASK FOR MAY 21, 2024:

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

Identify components with attributes that do not conform to HTML standard microsyntaxes. 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.
There are no attributes in Button.js file that use comma-separated tokens

maxatdetroit · 2024-05-21T15:30:06Z

@sreidthomas

Question 1:

If the component is not using slots are we still supposed to migrate to named slots?

No. If the component isn't using slots, then there is nothing to migrate from.

Question 2:

When you migrate to named slots are we only updating the template and story files? Is there always something specific that needs to be updated with each element besides the template and story files e.g. EventListeners?

Whenever you modify a component's public API (i.e. attribute name, attribute behavior, or component slots), we have to look for all the other places in the design system where that component is used. E.g. for cod-button do a code search for cod-button and see where the button is used in the design system. Anywhere it's used, we have to update which attributes and slots are used to match what we updated (e.g. if cod-modal is using cod-button it may still be using the data-close attribute which should be changed to just close).

Question 3:

When we migrate to named templates do we change how we add styles, varStyles, & bootstrapStyles to the component?

Do you mean named slots? If so, it depends. If the styling changes unexpectedly when we move the content into a slot, then yes, we might have to update the styles to target the elements correctly since they've moved in the component layout.

Question 4:

Are we only checking for the tasks with issue 201 in the .js files or the stories.js file too?

I think the answer to question 2 should answer this question as well but let me know if that's not clear.

sreidthomas · 2024-05-21T16:08:37Z

There are no slots in the Button.js file so Task 205: Migrate to named slots with basic markup where possible is not needed.

sreidthomas · 2024-05-21T16:37:13Z

NOTE TO SELF:

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

sreidthomas · 2024-07-11T17:07:33Z

Can you review this one for me and see if I'm on the right track? I think I have to make changes to the cod-icon in order for this to work correctly but I don't want to start on another component yet @maxatdetroit

maxatdetroit

Yep, definitely on the right track.

I think I have to make changes to the cod-icon in order for this to work correctly

That's not necessarily true. You could leave the cod-icon the same and handle it in a separate PR. That's what I'd recommend, because otherwise this PR will get pretty big. I left a comment about what I mean.

However, you will have to change all the places where cod-button gets used across the design system to use the correct attribute names that you changed. Here's a list of all the places where the button is used:

Places where it's created via HTML markup: https://github.com/search?q=repo%3ACityOfDetroit%2FCOD-Design-System+%3Ccod-button&type=code
Places where it's created via JS DOM API: https://github.com/search?q=repo%3ACityOfDetroit%2FCOD-Design-System+document.createElement%28%27cod-button%27&type=code

src/components/atoms/Button/Button.js

sreidthomas · 2024-07-12T00:20:47Z

Yep, definitely on the right track.

I think I have to make changes to the cod-icon in order for this to work correctly

That's not necessarily true. You could leave the cod-icon the same and handle it in a separate PR. That's what I'd recommend, because otherwise this PR will get pretty big. I left a comment about what I mean.

However, you will have to change all the places where cod-button gets used across the design system to use the correct attribute names that you changed. Here's a list of all the places where the button is used:

Places where it's created via HTML markup: https://github.com/search?q=repo%3ACityOfDetroit%2FCOD-Design-System+%3Ccod-button&type=code

Places where it's created via JS DOM API: https://github.com/search?q=repo%3ACityOfDetroit%2FCOD-Design-System+document.createElement%28%27cod-button%27&type=code

Yes that's what I meant starting another PR for it. But I see that I should keep moving with any components that are using the cod-button

sreidthomas · 2024-07-12T13:57:36Z

Yep, definitely on the right track.

I think I have to make changes to the cod-icon in order for this to work correctly

That's not necessarily true. You could leave the cod-icon the same and handle it in a separate PR. That's what I'd recommend, because otherwise this PR will get pretty big. I left a comment about what I mean.

However, you will have to change all the places where cod-button gets used across the design system to use the correct attribute names that you changed. Here's a list of all the places where the button is used:

Places where it's created via HTML markup: https://github.com/search?q=repo%3ACityOfDetroit%2FCOD-Design-System+%3Ccod-button&type=code

Places where it's created via JS DOM API: https://github.com/search?q=repo%3ACityOfDetroit%2FCOD-Design-System+document.createElement%28%27cod-button%27&type=code

You want me to change the places that are using the old attributes within this same PR @maxatdetroit?

maxatdetroit · 2024-07-12T15:39:35Z

Yes that's what I meant starting another PR for it. But I see that I should keep moving with any components that are using the cod-button

@sreidthomas ahh gotchya. Thanks for clarifying.

You want me to change the places that are using the old attributes within this same PR @maxatdetroit?

Yea, let's go ahead and do it in one PR. If we tried to do it in a separate PR, then it'd be difficult to merge the PRs in a safe way (that doesn't break the design system).

sreidthomas · 2024-08-02T15:40:09Z

I believe I covered everything but can you go over to make sure @maxatdetroit

maxatdetroit

It looks like we got most of the places where the <cod-button> is created via HTML, but not the places where the <cod-button> is created via JS DOM API.

Here are the other places we have to change: https://github.com/search?q=repo%3ACityOfDetroit%2FCOD-Design-System%20createElement(%27cod-button%27)&type=code

For example, the data-* attribute names are still used for the buttons here:

COD-Design-System/src/components/atoms/OffcanvasHeader/OffcanvasHeader.js

Lines 50 to 53 in cfd278c

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

src/stories/buttongroup.stories.js

src/stories/card.stories.js

sreidthomas · 2024-08-26T13:40:47Z

TO-DO:

Update attributes for these components/files:

maxatdetroit

I left a few comments where I noticed we are updating components incorrectly and conflating button component attributes with other components' attributes. I may have missed some places, so please take a look over the entire PR and see if I missed any similar places.

Also, please test your changes as you make them by viewing and interacting with the components you edit in storybook. I caught these issues because I tested this PR in storybook and noticed the Alert component is broken with these changes.

src/components/atoms/Alert/Alert.js

src/components/atoms/ModalFooter/ModalFooter.js

src/stories/buttongroup.stories.js

maxatdetroit

There's still a handful of places where this comment is still applicable:

I left a few comments where I noticed we are updating components incorrectly and conflating button component attributes with other components' attributes. I may have missed some places, so please take a look over the entire PR and see if I missed any similar places.

Please take another look.

src/components/atoms/ModalHeader/ModalHeader.js

src/components/atoms/OffcanvasHeader/OffcanvasHeader.js

src/components/organisms/Map/Map.js

maxatdetroit

Looks good now!

Btw, don't forget to set the base branch to 2.0.0-alpha1 for the uxds 2.0 PRs. I just updated this one on github.

sreidthomas · 2024-08-27T20:58:25Z

Going back over the code for this component and each task. For 201, we had to update everything to meet HTML Standard, and while studying, I learned that href is standard instead of using link like how we're doing. Or is there a reason why we're using link over href @maxatdetroit?

maxatdetroit · 2024-08-29T16:55:24Z

Going back over the code for this component and each task. For 201, we had to update everything to meet HTML Standard, and while studying, I learned that href is standard instead of using link like how we're doing. Or is there a reason why we're using link over href @maxatdetroit?

That's a really good catch @sreidthomas . There's no reason why we're using the link attr over href. Let's use href moving forward during the UXDS 2.0 migration.

Removed some data- from attributes

41bd315

sreidthomas added 2 commits May 21, 2024 09:12

Removed data- from id and aria-label

e64d7f3

Updated several custom attributes removing data-

0df3143

sreidthomas added 2 commits May 21, 2024 09:34

Updated data-disable to disabled

c2a9e20

Updated attributes in stories.js file

8e5ec8b

sreidthomas requested a review from maxatdetroit July 11, 2024 17:07

maxatdetroit reviewed Jul 11, 2024

View reviewed changes

src/components/atoms/Button/Button.js Outdated Show resolved Hide resolved

Changed attributes for cod-icon back to the original

04f101e

sreidthomas added 11 commits July 15, 2024 14:51

Updated attributes in Basic example

5d38f3a

Updated attributes in variations example

b4d4094

Updated attributes in Split example

b27ceb8

Updated extra-classes attribute in Basic, Variations and Split examples

67fb47f

Updated attributes in DarkMode example

daa5871

Updated attributes in Basic example

8dbc376

Updated attributes in Brand example

b913c18

Updated attributes in Offcanvas example

b1fe835

Updated attributes in OffcanvasResponsive example

b039fa6

Updated attributes in OffcanvasColor example

4972f93

Updated attributes in cod-button for Form Template

b30fd0a

sreidthomas added 4 commits July 19, 2024 14:55

Fixed prettier issues in 1 file

e721dc4

Changed close, primary and disableStatus to hasAttribute

48adb8d

Updated primary logic to not include true/false

8aecc85

Updated close logic and removed ternary

7db74a7

sreidthomas requested a review from maxatdetroit August 2, 2024 15:39

maxatdetroit requested changes Aug 5, 2024

View reviewed changes

src/stories/buttongroup.stories.js Outdated Show resolved Hide resolved

src/stories/card.stories.js Outdated Show resolved Hide resolved

sreidthomas added 2 commits August 26, 2024 09:34

Fixed attribute in card.stories.js

a390d1f

Fixed attribute in buttongroup.stories.js file

2bb7ad3

sreidthomas added 5 commits August 26, 2024 09:46

Updated attributes in Alert.js file

8cf9dad

Updated attributes in ModalFooter.js

8cbbad8

Updated attributes in Map.js file

8883951

Updated attributes in OffcanvasHeader.js

a133e0f

Updated attributes in ModalHeader.js

e967a55

maxatdetroit requested changes Aug 26, 2024

View reviewed changes

src/components/atoms/Alert/Alert.js Outdated Show resolved Hide resolved

src/components/atoms/ModalFooter/ModalFooter.js Outdated Show resolved Hide resolved

src/stories/buttongroup.stories.js Outdated Show resolved Hide resolved

sreidthomas added 3 commits August 27, 2024 11:11

Changed attributes in Alert.js back to normal

9ab6e2e

Fixed attributes in buttongroup.stories.js file

0184e92

Fixed attributes in ModalFooter.js file

6914ffe

sreidthomas requested a review from maxatdetroit August 27, 2024 15:18

maxatdetroit requested changes Aug 27, 2024

View reviewed changes

src/components/atoms/ModalHeader/ModalHeader.js Outdated Show resolved Hide resolved

src/components/atoms/OffcanvasHeader/OffcanvasHeader.js Outdated Show resolved Hide resolved

src/components/organisms/Map/Map.js Show resolved Hide resolved

sreidthomas added 4 commits August 27, 2024 12:42

Added data back to bs-dimsmis

a29334e

fixed extra classes attribute in ModalHeader.js file

412bf1c

Added data back to bs-dismiss attribute

f7fc672

Fixed data-extra-classes attribute in OffcanvasHeader.js file

aa52347

maxatdetroit changed the base branch from dev to 2.0.0-alpha1 August 27, 2024 16:51

maxatdetroit approved these changes Aug 27, 2024

View reviewed changes

maxatdetroit merged commit 0b8f642 into 2.0.0-alpha1 Aug 29, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sreidthomas commented May 20, 2024

sreidthomas commented May 20, 2024 •

edited

Loading

sreidthomas commented May 21, 2024 •

edited

Loading

sreidthomas commented May 21, 2024 •

edited

Loading

sreidthomas commented May 21, 2024 •

edited

Loading

maxatdetroit commented May 21, 2024

sreidthomas commented May 21, 2024

sreidthomas commented May 21, 2024

sreidthomas commented Jul 11, 2024

maxatdetroit left a comment

sreidthomas commented Jul 12, 2024

sreidthomas commented Jul 12, 2024

maxatdetroit commented Jul 12, 2024

sreidthomas commented Aug 2, 2024

maxatdetroit left a comment

sreidthomas commented Aug 26, 2024 •

edited

Loading

maxatdetroit left a comment

maxatdetroit left a comment

maxatdetroit left a comment

sreidthomas commented Aug 27, 2024

maxatdetroit commented Aug 29, 2024

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

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

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

Conversation

sreidthomas commented May 20, 2024

WHAT WAS DONE:

OVERALL TASK / JOB DUTIES:

Breakdown of individual tasks for each issue:

sreidthomas commented May 20, 2024 • edited Loading

UPDATE AS OF MAY 20, 2024:

Task 202: Simplify attribute naming conventions part 1 & 2

Task 202: Simplify attribute naming conventions part 3

sreidthomas commented May 21, 2024 • edited Loading

QUESTIONS: @maxatdetroit

sreidthomas commented May 21, 2024 • edited Loading

CURRENT ISSUES AS OF MAY 21, 2024:

DO THIS ONCE YOU GET TO COD-ICON PR

sreidthomas commented May 21, 2024 • edited Loading

NEW TASK FOR MAY 21, 2024:

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

maxatdetroit commented May 21, 2024

sreidthomas commented May 21, 2024

There are no slots in the Button.js file so Task 205: Migrate to named slots with basic markup where possible is not needed.

sreidthomas commented May 21, 2024

sreidthomas commented Jul 11, 2024

maxatdetroit left a comment

Choose a reason for hiding this comment

sreidthomas commented Jul 12, 2024

sreidthomas commented Jul 12, 2024

maxatdetroit commented Jul 12, 2024

sreidthomas commented Aug 2, 2024

maxatdetroit left a comment

Choose a reason for hiding this comment

sreidthomas commented Aug 26, 2024 • edited Loading

TO-DO:

maxatdetroit left a comment

Choose a reason for hiding this comment

maxatdetroit left a comment

Choose a reason for hiding this comment

maxatdetroit left a comment

Choose a reason for hiding this comment

sreidthomas commented Aug 27, 2024

maxatdetroit commented Aug 29, 2024

sreidthomas commented May 20, 2024 •

edited

Loading

sreidthomas commented May 21, 2024 •

edited

Loading

sreidthomas commented May 21, 2024 •

edited

Loading

sreidthomas commented May 21, 2024 •

edited

Loading

sreidthomas commented Aug 26, 2024 •

edited

Loading