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

(web-components) Use ElementInternals for Button elements #30999

Merged

Conversation

radium-v
Copy link
Contributor

@radium-v radium-v commented Apr 8, 2024

Previous Behavior

Currently, form-associated custom elements use the FormAssociated class mixin. This mixin is heavy and duplicative, being copied wholesale for every component which utilizes it. While FormAssociated includes minimal support for ElementInternals, the implementation isn't fully baked.

New Behavior

Improvements:

  • Switches the <fluent-button> custom element to use ElementInternals by default. An optional polyfill may be added in the near future, as more components transition to use ElementInternals.
  • Removes the checkbox-like implementation from <fluent-toggle-button> in favor of leaning on aria-pressed (an element with a role of button and the aria-pressed attribute is treated as a toggle button).
  • Adds Playwright tests for the Button element.
  • Synchronizes the <fluent-menu-button> and <fluent-compound-button> elements to work with the changes in <fluent-button>, since their classes are extensions of Button.
  • Copies the original button styles into the stylesheet for <fluent-anchor-button>. The AnchorButton class isn't actually extended from Button but it did use the same stylesheet. Its template structure hasn't changed.
  • Adds a story for submit- and reset-type buttons to the Button story file.

Other fixes:

  • The Playwright config had a few minor misconfigurations:
    • The projects field was set to run all tests in Chromium three times in a row.
    • The viewport width and height values were flipped.

@radium-v radium-v requested review from a team as code owners April 8, 2024 19:55
@radium-v radium-v force-pushed the users/radium-v/web-components-v3 branch 2 times, most recently from 5dd4204 to ef03c6a Compare April 9, 2024 00:32
Copy link

codesandbox-ci bot commented Apr 9, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@radium-v radium-v force-pushed the users/radium-v/web-components-v3 branch from 68ef4f9 to e46ddf2 Compare April 9, 2024 02:19
@tudorpopams tudorpopams requested a review from Hotell April 9, 2024 11:52
@Hotell
Copy link
Contributor

Hotell commented Apr 9, 2024

Some workflow notes:

bumping deps and adding prodcution changes should be ideally done in N separate PRs as it's unrelated.

this would be more likely a blocker if this PR would be against master branch but for you case I leave that decision up to you as it doesn't effect daily monorepo contributions.

@@ -29,6 +29,9 @@ jobs:
filePath: yarn-ci.sh
displayName: yarn (install packages)

- script: yarn playwright install --with-deps
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI:

this is ok to live in wc3 branch.

if this would be against master we would most likely not accept this, as the cost of prolonging pipeline just for sake of 1 package is not worth the slowdown. If whole monorepo moves to use playwright for all e2e in feature then it's totally valid.

For this use-case we would need to make this on-demand as we do within fluent-contrib https://github.com/microsoft/fluentui-contrib/blob/main/packages/nx-plugin/src/executors/playwright/executor.ts#L51-L54

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are the Playwright binaries currently made available for existing packages which use Storywright?

"paths": {}
}
}
"tsconfigFilePath": "./tsconfig.api-extractor.json"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this change necessary ? another exotic tsconfig === more confusion / not following repo patterns

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API Extractor doesn't use the tsconfigFilePath if any overrideTsconfig fields are present. This was causing warnings "Subsequent property declarations must have the same type" (TS2717) with @types/web present.

Copy link
Contributor

@Hotell Hotell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approving as it's for wc3 branch. these changes would not be acceptable for master - added more context in comments for better understanding. ty

@radium-v radium-v force-pushed the users/radium-v/web-components-v3 branch from 797dc64 to cdad9b4 Compare April 18, 2024 00:41
@radium-v radium-v changed the title (web-components) Use ElementInternals for Button element (web-components) Use ElementInternals for Button elements Apr 18, 2024
@radium-v radium-v marked this pull request as ready for review April 18, 2024 00:50
@radium-v radium-v requested a review from Hotell April 18, 2024 00:52
@chrisdholt chrisdholt merged commit 2eab460 into microsoft:web-components-v3 Apr 22, 2024
11 checks passed
radium-v added a commit to radium-v/fluentui that referenced this pull request Apr 29, 2024
radium-v added a commit to radium-v/fluentui that referenced this pull request Apr 29, 2024
radium-v added a commit to radium-v/fluentui that referenced this pull request Apr 30, 2024
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.

None yet

4 participants