ChoiceGroup: getStyles conversion by FalseLobster · Pull Request #4852 · microsoft/fluentui

FalseLobster · 2018-05-11T20:24:27Z

Pull request checklist

Addresses an existing issue: Fixes Allow customization of ChoiceGroup styles #3872
Include a change request file using $ npm run change

Description of changes

Splits ChoiceGroup into two components, ChoiceGroup and ChoiceGroupOption
Migrates from scss styling to javascript-based getStyles
Adds support for conditionally applying global class names

Focus areas to test

ChoiceGroup visuals

… split out ChoiceGroupOption into subcomponent. Everything working except Focus rect right now

…c-react into ChoiceGroup # Conflicts: # packages/office-ui-fabric-react/src/components/ChoiceGroup/ChoiceGroup.tsx # packages/office-ui-fabric-react/src/components/ChoiceGroup/ChoiceGroup.types.ts

…c-react into ChoiceGroup

FalseLobster · 2018-05-11T20:31:31Z

package.json

    "@microsoft/rush": "4.3.0"
+  },
+  "dependencies": {
+    "npm": "^6.0.0"


This is actually unintentional. npm told me at some point to run a command, which I diligently obeyed, and this edit got auto-added. Commentary is welcome on why npm thought this was necessary... ?

I believe npm was telling you that a new version of npm is available and prompting you to upgrade :) But.. you might have entered incorrectly (specifically, missed the -g option so it became a local install that was written to package.json)

You probably want to revert this.

FalseLobster · 2018-05-11T20:36:55Z

packages/office-ui-fabric-react/src/components/ChoiceGroupOption/ChoiceGroupOption.base.tsx

+      <div className={ this._classNames.root }>
+        <div className={ this._classNames.choiceFieldWrapper }>
+          <input
+            { ...getNativeProps(this.props, inputProperties) }


My understanding here is that stomping over getNativeProps is undesirable and the spread should really be at the end of the tag. I ran into issues with that ordering, but I want to leave this comment here to solicit commentary and to remind myself to revisit whether or not I'm still seeing issues with this ordering.

FalseLobster · 2018-05-11T20:39:32Z

packages/office-ui-fabric-react/src/components/ChoiceGroupOption/ChoiceGroupOption.test.tsx

+import { ChoiceGroupOption } from './ChoiceGroupOption';
+
+describe('ChoiceGroupOption', () => {
+


I don't know if there's a best practice for authoring snapshot tests, so I just picked a bunch of interesting permutations of the ChoiceGroupOption props.

cliffkoh

Going to close/reopen the PR to force a re-base

cliffkoh · 2018-05-14T19:52:25Z

common/changes/office-ui-fabric-react/ChoiceGroup_2018-05-11-20-21.json

    {
      "packageName": "office-ui-fabric-react",
-      "comment": "Deprecate Persona's primaryText prop and add replacement text prop.",
+      "comment": "ChoiceGroup getStyles conversion",


As we are targeting the 6.0 branch, we should rename all the changefiles to 6_0_choiceGroupGetStyles.json manually

FalseLobster · 2018-05-15T21:15:39Z

Issue #4836 should now also be addressed by this PR

srideshpande · 2018-05-17T20:41:39Z

packages/office-ui-fabric-react/src/components/ChoiceGroup/ChoiceGroup.types.ts

+  theme?: ITheme;
+
+  /**
+   * Call to provide customized styling that will layeron top of the variant rules.


layeron [](start = 50, length = 7)

layer on

srideshpande · 2018-05-17T20:49:19Z

packages/office-ui-fabric-react/src/components/ChoiceGroupOption/ChoiceGroupOption.types.ts

+  theme?: ITheme;
+
+  /**
+   * Call to provide customized styling that will layeron top of the variant rules.


layeron [](start = 50, length = 7)

layer on

srideshpande · 2018-05-17T20:56:37Z

packages/office-ui-fabric-react/src/components/ChoiceGroupOption/index.ts

@@ -0,0 +1,2 @@
+export * from './ChoiceGroupOption';
+export * from './ChoiceGroupOption.types';


why is ChoiceGroupOption at components level? can this be used as an independent component outside of ChoiceGroup? if not, can you put it under ChoiceGroup folder?

I think there is value in letting this be used as an independent component, though that might be good to do at a later date once everyone is happy with this current abstraction.

srideshpande · 2018-05-17T21:06:42Z

packages/office-ui-fabric-react/src/components/ChoiceGroup/ChoiceGroup.types.ts

+  /**
+   * Call to provide customized styling that will layeron top of the variant rules.
+   */
+  getStyles?: IStyleFunction<IChoiceGroupStyleProps, IChoiceGroupStyles>;


getStyles? [](start = 2, length = 10)

Since you are introducing it here, it would be nice to have an example around its usage in the example pages we have for ChoiceGroup.

In 6.0 we're decided to stick with "styles" as the prop name, and allow it to support a function or simple style object as a value. Can we get inline with that approach?

micahgodbolt · 2018-05-17T21:19:32Z

packages/office-ui-fabric-react/src/components/ChoiceGroupOption/ChoiceGroupOption.types.ts

+  /**
+   * Call to provide customized styling that will layeron top of the variant rules.
+   */
+  getStyles?: IStyleFunction<IChoiceGroupOptionStyleProps, IChoiceGroupOptionStyles>;


same here...new 'styles' approach set to IStyleFunction<> || IChoiceGroupOptionStyles

micahgodbolt

I'd like to hold this PR until we resolve styles vs getStyles. I'm also curious if optionAs needs to be used as well. @dzearing i noticed crumbAs was eventually removed, is this still a pattern we want to follow?

JasonGore · 2018-05-18T18:35:59Z

Per our discussion, this PR should make sure the IChoiceGroupOption "output" props labelId, checked, and id are still working properly. I will make a separate issue to deal with removing the "output" props so that props no longer have to be mutated.

cliffkoh · 2018-05-18T19:20:28Z

Hi @micahgodbolt, as the change targets 5.0, she should check in getStyles. When we do the merge into 6.0 though, we'll rename on @FalseLobster 's behalf to styles.

FalseLobster · 2018-05-18T21:24:59Z

Hey all. As far as I know the only outstanding issue is the concern raised by @JasonGore regarding the mutation of output props. I think that some sort of "breaking" design change here may be necessary, either in the IChoiceGroupOption we pass in as argument to the callbacks or the mutating behavior. I'll follow up later with more on this point, but I wanted to at least commit the move of CGO to a subdirectory of CG.

JasonGore · 2018-05-18T21:49:22Z

I broke the output props as a separate issue because I was fairly certain that, even though using output props is unorthodox, we probably don't want to yank them out entirely on 5.0 or without deprecating them first. I think the best course of action is to have this PR continue providing those props unless this is prohibitive to do in this refactor. (In which case we can discuss how to introduce breaking changes, whether by targeting 6.0 or some other method.)

#4962) * Revert "ChoiceGroup: getStyles conversion (#4852)" This reverts commit d152bb7. * Adding change files.

@cschleiden

* master: (67 commits) Applying package updates. Revert ChoiceGroup change in 5.0 to minimize potential partner impact. (microsoft#4962) Applying package updates. ChoiceGroup: getStyles conversion (microsoft#4852) Export SASS variables and mixins (microsoft#4959) Variants: update algorithm (microsoft#4949) Allow for customization of keycodes that cause the focus rect to appear (microsoft#4948) Mergestyles facepile (microsoft#4950) Fixing circular dependency and non-AMD references in ContextualMenu (microsoft#4946) Clean up semantic slots (microsoft#4932) Added missing merge-styles background-size typing (microsoft#4935) Applying package updates. Split menu button styles (microsoft#4922) Experimental Chiclet Component (microsoft#4678) Remove hover and pressed background colors (microsoft#4908) Remove @cschleiden as Rating code owner (microsoft#4929) Addressing Issue microsoft#3691 - SplitButton: In Safari, the SplitButton has mismatched heights (microsoft#4797) Applying package updates. Adding option for focus on tags in disabled picker (microsoft#4833) Jest merge-styles serialization fix for animation-name (microsoft#4927) ...

Alicia Drummond added 8 commits May 4, 2018 11:40

ChoiceGroup - Moving over javascript based getStyles instead of scss,…

3c8e03d

… split out ChoiceGroupOption into subcomponent. Everything working except Focus rect right now

Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…

d96b05b

…c-react into ChoiceGroup

Fixes minor visual, tslint errors. Tests still broken.

643a80f

conditionally apply global class names, fix up tests

20e4607

minor fixes, snapshot test for ChoiceGroupOption

da2b46e

Merge branch 'master' into ChoiceGroup

5145e28

npm run change

f0852d6

FalseLobster requested a review from srideshpande as a code owner May 11, 2018 20:24

FalseLobster commented May 11, 2018

View reviewed changes

cliffkoh changed the base branch from master to new_6.0 May 14, 2018 19:46

cliffkoh reviewed May 14, 2018

View reviewed changes

cliffkoh closed this May 14, 2018

cliffkoh reopened this May 14, 2018

cliffkoh changed the base branch from new_6.0 to master May 14, 2018 22:13

cliffkoh changed the base branch from master to new_6.0 May 14, 2018 22:13

Remove unintended change to package.json, fix screener test

02cf116

dzearing changed the title ~~Choice group getStyles conversion~~ ChoiceGroup: getStyles conversion May 15, 2018

FalseLobster changed the base branch from new_6.0 to master May 15, 2018 20:32

Alicia Drummond added 2 commits May 15, 2018 13:50

Reordered nativeprops for parity with original, fixed aria-labeledby bug

69d9d28

Update snapshot to account for new aria-labelledBy behavior

1cc6cbe

dzearing added 2 commits May 16, 2018 22:19

Update ChoiceGroup_2018-05-11-20-21.json

a4d430f

Update ChoiceGroup_2018-05-11-20-21.json

64bd7bd

dzearing approved these changes May 17, 2018

View reviewed changes

richardtagger mentioned this pull request May 17, 2018

feat(choicegroup): Initial ChoiceGroup Implementation microsoft/YamUI#340

Merged

2 tasks

JasonGore approved these changes May 17, 2018

View reviewed changes

srideshpande reviewed May 17, 2018

View reviewed changes

micahgodbolt reviewed May 17, 2018

View reviewed changes

micahgodbolt requested changes May 17, 2018

View reviewed changes

JasonGore mentioned this pull request May 17, 2018

TooltipHost and ChoiceGroup components mutate props #4715

Closed

dzearing added 2 commits May 17, 2018 17:33

Update ChoiceGroup.types.ts

73a21f4

Update ChoiceGroupOption.types.ts

bd57d47

JasonGore mentioned this pull request May 18, 2018

ChoiceGroup has "output" props #4918

Closed

Move ChoiceGroupOption into subdirectory of ChoiceGroup.

802523a

micahgodbolt approved these changes May 21, 2018

View reviewed changes

dzearing approved these changes May 23, 2018

View reviewed changes

dzearing merged commit d152bb7 into microsoft:master May 23, 2018

dzearing added a commit that referenced this pull request May 23, 2018

Revert ChoiceGroup change in 5.0 to minimize potential partner impact. (

17547b3

#4962) * Revert "ChoiceGroup: getStyles conversion (#4852)" This reverts commit d152bb7. * Adding change files.

JasonGore mentioned this pull request May 23, 2018

ChoiceGroup: getStyles conversion #4964

Merged

2 tasks

microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019

		import { ChoiceGroupOption } from './ChoiceGroupOption';

		describe('ChoiceGroupOption', () => {

		@@ -0,0 +1,2 @@
		export * from './ChoiceGroupOption';
		export * from './ChoiceGroupOption.types';

Conversation

FalseLobster commented May 11, 2018

Pull request checklist

Description of changes

Focus areas to test

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cliffkoh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FalseLobster commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

micahgodbolt left a comment

Choose a reason for hiding this comment

Uh oh!

JasonGore commented May 18, 2018

Uh oh!

cliffkoh commented May 18, 2018

Uh oh!

FalseLobster commented May 18, 2018

Uh oh!

JasonGore commented May 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

JasonGore commented May 18, 2018 •

edited

Loading