-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
docs(rfcs): add triage automation rfc #24817
Conversation
📊 Bundle size report🤖 This report was generated against 64fb6212cd2f12d40f10f16d0154b5786c3534c6 |
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. Latest deployment of this branch, based on commit d9b832f:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 64fb6212cd2f12d40f10f16d0154b5786c3534c6 (build) |
|
||
> Note: | ||
> | ||
> - by "project" we mean one of libraries that exist within monorepo (v0,v8,v9,web-components) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this make sense with respect to the current expectations for partners ? We actively encourage partners to use @fluentui/react-components
. Using package labels instead of component labels seems like we are fighting against our own recommendation to use the suite package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure I understand the concern?
- by "project" we mean one of libraries that exist within monorepo (v0,v8,v9,web-components)
we do this today - add label based on project , NOT granular package.
probably you're asking about option 2 (2. package label assignment
)?
can you pls clarify? ty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I 'm asking about packge label assignment
why should we oblige users to select the correct package in our monorepo when the only exposure they have (and we do it intentionally) is the library package ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
answered with proposal #24817 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated RFC d9b832f
Based on selected project within issue template, additional select should be dynamically generated so contributor can pick proper package. | ||
|
||
<img width="569" alt="example of flow" src="https://user-images.githubusercontent.com/1223799/190431945-56fcd343-6eb7-4145-b2b2-62f5919b8c31.png"> | ||
|
||
Unfortunately **[dynamic input controls are not possible with github issues beta](https://github.com/community/community/discussions/29067)** which leaves us with following options how to implement this: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this allow you to select multiple packages? Or in case the issue affected multiple packages, how would the scenario proceed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question, we can enable multiple items selected with github forms https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/syntax-for-githubs-form-schema#dropdown.
What would be the use-case ?
to provide more context:
Our template is aimed mainly for 3rd party with workflow.
Meaning: "Im using fluent, Im using this component and it doesn't work for me."
From my experience user reports issue for the immediate thing that comes in his way( component A ).
Additionally one can dig deeper and find that the issue is within some underlying component (Component B) - thus the reporter should pick that one.
In general having fine grained issue reports reduces the complexity and time to identify the problem. This is omnipresent everywhere - dissecting task to smaller ones/ atomic commits/small PR's.
to wrap up I'm IMO it shouldn't be needed. but as mentioned the option is there
subgraph "parse codeowners" | ||
C-->D("find owner based on package") | ||
end | ||
D-->A("add issue to unified board")-->AA("pick team based on ownership definition"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The role of the shield is to:
- Verify the issue is actionable.
- Set the priority for the issue.
In order to be able to do that, shield often needs to talk back to the reporter and/or debug the issue. As a side effect, the person on shield widen their knowledge to cover the whole library (useful for both support and development).
If the bot assigns the issues automatically, then:
- We are siloing crews.
- We require each crew to do their own "shield" (not necessarily having a dedicated person) to cover the two points above on a daily basis.
For that reason I would prefer the bot to suggest the owner but would still let shield to the check/assignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For that reason I would prefer the bot to suggest the owner but would still let shield to the check/assignment.
that's fine, it will still contain "needs triage" label, so person on shield can double check. we can create a view in project board that will show all "need triage" items for better visibility and post-bot processing.
WDYT ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me. I still think that shield is valuable and should visit every incoming issue for triage/investigation.
What you are proposing is achieving that while reducing manual repetitive work = win-win
- pros | ||
- only 1 issue/feature template that we need to maintain | ||
- removes need of `1. project label assignment` as labeling would be handled in one step | ||
- creating feature/bug templates per library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote for template per library.
Simple to do while allowing us to have each template suit the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated d9b832f
|
||
Based on selected project within issue template, additional select should be dynamically generated so contributor can pick proper package. | ||
|
||
<img width="569" alt="example of flow" src="https://user-images.githubusercontent.com/1223799/190431945-56fcd343-6eb7-4145-b2b2-62f5919b8c31.png"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @ling1726 partners are supposed to use just the suite package, we should not ask them to guess the individual package.
Can we change this to list of components instead of packages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this to list of components instead of packages?
We can, but it might be impossible to identify those as packages export various Controls.
Two approaches come to my mind:
- use storybooks root chapters for those
- Pascal Casing package names without scope
- robust and easy to sync
- Example:
@fluentui/react-text
->Text
,@fluentui/react-menu
->Menu
I'd recommend to go with 2), WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
option 2 good enough 👍
3. issue origin: v0 template: | ||
|
||
``` | ||
Text -> package: react-northstar, Fluent UI react-northstar (v0) | ||
Menu -> package: react-northstar, Fluent UI react-northstar (v0) | ||
Dialog -> package: react-northstar, Fluent UI react-northstar (v0) | ||
``` | ||
|
||
4. issue origin: web-components template: | ||
|
||
``` | ||
Text -> package: web-components | ||
Menu -> package: web-components | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For cases 3. and 4. above, will we still have a label indicating the component? These have been pretty helpful for v8.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- web-components doesn't use this https://github.com/microsoft/fluentui/issues?q=is%3Aissue+label%3Aweb-components
- v8, I think we can create pluggable architecture for the bot so these specific use cases can be handled solely for
@fluentui/react
. Do those labels mirror sub-folders or what is the logic ?
2. creating feature/bug templates per library | ||
|
||
- cons | ||
- 2 (feature/bug) x N templates (N is number of libraries in monorepo) | ||
- pros | ||
- clear separation that can be tweaked to particular library needs if needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 vote for option 2
This breaks the decision up for contributors into phases
- Decide the library choose a template (should be easy)
- As part of filing choose a component from a smaller list (easier or the same as other options)
I also think that the likelihood of having differences between library template questions is high, so having separate templates for each will have other benefits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I also like option 2.
One question: will the dropdown include options for issues that don't directly relate to a component? (e.g. an issue with slots in general, or a problem integrating into their library or something?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would go for the option 2 with something other than package names since it goes against our direct guidance to use these packages.
Great question ! this sounds to me like a need for another template as it wont match the purpose - component issue/feature ( maybe this will push us to finally come up with thoughts ? |
* stress-test: add "afterframe" dependency (microsoft#25560) Previously stress-test used a "requestPostAnimationFrame" function that is used to measure from just before style recalc, layout and paint are executed to just afterward. This change drops that function in favor of taking a dependency on afterframe, a library that does the same thing. * Fix: Update tabs to remain the same size between unselected and selected states (microsoft#25542) * Add support to keep tabs from changing size when selected * Update for naming and tests * Rename update * Update packages/react-components/react-tabs/src/components/TabList/TabList.types.ts Co-authored-by: Sean Monahan <[email protected]> Co-authored-by: Sean Monahan <[email protected]> * chore: Add reduced-motion documentation to Spinner (microsoft#25561) * chore: Add reduced-motion documentation to Spinner * change file * chore: Add documentation to Progress about reduced-motion (microsoft#25563) * chore: Add documentation to Progress about reduced-motion * change file * stress-test: disable CSS transitions on injected styles (microsoft#25559) This addresses an issue where Firefox's style recalculation measurement becomes unstable when controls under test have CSS transitions. Specifically, our test injects styles that update the background color of selected controls and many Fluent controls transition the background color for different states like "hover" and "focus". With transitions running Firefox will sometimes include the transition time in the style recalc measurement and sometimes not. Additionally, Chromium browsers never include the transition time in the measurement. Disabling transitions ensures measurement behavior is consistent in Firefox and in alignment with the behavior of Chromium browsers. * docs(rfcs): add triage automation rfc (microsoft#24817) * chore: update beachball ignore list to apply to new project structure (microsoft#25531) * bugfix(react-switch): adds line-height=0 to switch indicator slot (microsoft#25507) * chore: add both options to useArrowNavigationGroup (microsoft#25568) * chore: add both options to useArrowNavigationGroup * chore: add changes * chore: update api * chore(vr-tests-v9): Convert Dialog and Image VR tests to CSF (microsoft#25527) * chore: toolbar a11y improvements (microsoft#25562) * feat: create vertical example * chore: add navigation arrows for vertical scenario * chore: add labels for all toolbar stories * chore: update tooltip example * chore: update radio example * chore: update toolbar stories examples * chore: add changes * chore: use both in toolbar arrow key nav * chore: update snapshot * chore(react-dialog): migrate to new package structure (microsoft#25523) Co-authored-by: Oleksandr Fediashov <[email protected]> * chore(react-overflow): migrate to new package structure (microsoft#25524) * Expand @fluentui/react root index file's export *s to uncover and fix duplicate exports. (microsoft#25545) * expanding index. * change. * Replacing with inline deprecation exclusions. * setting export * to warning. * change. * moving rules around. * fix: v9 form controls with underline should have underline-specific disabled styling (microsoft#25543) Updates disabled form styling for Input, Select, Spinbutton, Combobox, and Dropdown * docs(public-docsite-v9): Adding a basic setup section to the SSR docs (microsoft#25564) * applying package updates * update regex * updatE * Update azure-pipelines.vrt-baseline.yml for Azure Pipelines * Update azure-pipelines.vrt-baseline.yml for Azure Pipelines * chore(deps): bump loader-utils from 2.0.0 to 2.0.3 (microsoft#25567) Bumps [loader-utils](https://github.com/webpack/loader-utils) from 2.0.0 to 2.0.3. - [Release notes](https://github.com/webpack/loader-utils/releases) - [Changelog](https://github.com/webpack/loader-utils/blob/v2.0.3/CHANGELOG.md) - [Commits](webpack/loader-utils@v2.0.0...v2.0.3) --- updated-dependencies: - dependency-name: loader-utils dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore: migrate @types/node to 14 to align with our supported node version (microsoft#25510) * chore(deps): resolve only to 1 @types/node version to mitigate global leaks and api-generation errors * fix(bundle-size): fix type errors exposed by node 14 typings * fix(tools): fix type errors exposed by node 14 typings * fix(typings): fix type errors exposed by node 14 typings * revert: use resolution for @types/node * chore(ts-minbar-test-react): use @types/node 14 explicitly * generate changefiles * chore: dedupe deps * fix(scripts): fix type errors exposed by node 14 typings * test: replace deprecated module.parent with require.main within isConformance * generate changefiles * test(react): clean up persona-coin test and use isConformant without side-effects * fix(storybook): fix theme picker current selection (microsoft#25533) * update * chore(vr-tests-v9): Convert Accordion VR tests to CSF (microsoft#25525) * fix: Update Avatar active ring color to match base color (microsoft#25497) * RFC: Field Package Layout (microsoft#25380) * Feature: Added large tab size to react-tabs (microsoft#25577) * Added large tab size * yarn change * Update vr-tests * Code review fix * feat(react-infobutton): Adding size prop, HCM styles, and updating styles to match design spec (microsoft#25519) * updating styles and adding size prop * updating comment * adding requested changes * fix: Adding expanded styles for MenuButtons and making various other styling fixes for Button components (microsoft#25521) * fix: Adding expanded styles for MenuButtons and making various other styling fixes for Button components. * Adding change file. * Addressing PR feedback. Co-authored-by: KHMakoto <[email protected]> * chore: Clean up Field tests and story imports in preparation of moving to individual packages (microsoft#25594) * Remove custom `isConformant` function for Field tests, and instead inline the customizations in each call to `isConformant` * Disable the `exported-top-level` test because the components will be exported as e.g. `InputField_unstable` from the component packages. * Change the stories to import from `@fluentui/react-components/unstable`, instead of the individual package. * Rename the `FieldComponent` type to `FieldControl` to correspond to the `control` slot name. * Tabs icon toggle (microsoft#25597) * Added regular filled icon toggling * yarn change * chore: Refactor Field components into the base component's package (microsoft#25593) Move Field components into their respective packages, as discussed in RFC microsoft#25380 * Update Component Implementation Epic template (microsoft#25480) * update to use vrscreenshotdiff beta * change lock file * update package version * applying package updates * update pr pipeline to add v8 * PR to add tasks in pipeline for v9 VRT integration (microsoft#25606) * fix: create valid export maps (microsoft#25558) * generate changefiles * chore(deps): bump socket.io-parser from 4.0.4 to 4.0.5 (microsoft#25604) Bumps [socket.io-parser](https://github.com/socketio/socket.io-parser) from 4.0.4 to 4.0.5. - [Release notes](https://github.com/socketio/socket.io-parser/releases) - [Changelog](https://github.com/socketio/socket.io-parser/blob/main/CHANGELOG.md) - [Commits](socketio/socket.io-parser@4.0.4...4.0.5) --- updated-dependencies: - dependency-name: socket.io-parser dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(react-dialog): removes unnecessary union case for DialogOpenChangeData (microsoft#25504) * templatize * update * templatize * updatE * refactor(scripts): remove deprecated exec abstractions (microsoft#25569) * fix(scripts): make eslint run again on pre-commit (microsoft#25537) * feat(react-components): Move AvatarGroup to stable (microsoft#25005) * update * update version * run partial tests * update * update * update * update * update * update * updatE * updatE * update version * update lock file * update name * update version * add quotes * docs: refactor Text documentation and add missing guidance for presets/alignment (microsoft#25587) Fixes microsoft#24341 Fixes microsoft#25548 * update * package version * fix(react-menu): remove unwanted aria attributes on context menu (microsoft#25615) * fix(react-menu): remove unwanted aria attributes on context menu * chore: updates trigger selector for cypress tests * remove unwanted param * update * applying package updates * convert * checkpoint * add regex * change convert script * updates * update sw version Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Sean Monahan <[email protected]> Co-authored-by: Geoff Cox (Microsoft) <[email protected]> Co-authored-by: tomi-msft <[email protected]> Co-authored-by: Martin Hochel <[email protected]> Co-authored-by: Bernardo Sunderhus <[email protected]> Co-authored-by: chajun <[email protected]> Co-authored-by: Tristan Watanabe <[email protected]> Co-authored-by: Oleksandr Fediashov <[email protected]> Co-authored-by: David Zearing <[email protected]> Co-authored-by: Sarah Higley <[email protected]> Co-authored-by: Esteban Munoz Facusse <[email protected]> Co-authored-by: Fluent UI Build <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Tudor Popa <[email protected]> Co-authored-by: Ben Howell <[email protected]> Co-authored-by: Makoto Morimoto <[email protected]> Co-authored-by: KHMakoto <[email protected]> Co-authored-by: Marcos Moura <[email protected]>
👓 PREVIEW