Skip to content

Uniformly export component "Base" implementations#5807

Merged
cliffkoh merged 5 commits intomicrosoft:masterfrom
KevinTCoughlin:keco/export-base-impls
Aug 11, 2018
Merged

Uniformly export component "Base" implementations#5807
cliffkoh merged 5 commits intomicrosoft:masterfrom
KevinTCoughlin:keco/export-base-impls

Conversation

@KevinTCoughlin
Copy link
Copy Markdown
Member

@KevinTCoughlin KevinTCoughlin commented Aug 4, 2018

Pull request checklist

Description of changes

Tackled this as I'm waiting to go over the contextual menu changes with Micah. I audited the components and added all missing "Base" implementations I could find. There were also some .types exports missing that are probably necessary to theme the components successfully.

Remaining non-exported components that I was unsure about w.r.t API surface:

  • CalloutContent.base
  • ChoiceGroupOption & ChoiceGroupOption.base
  • ColorRectangle & ColorSlider
  • DetailsHeader.base
  • GroupHeader.base / GroupFooter.base / GroupShowAll.base
  • KeyTipContent.base
  • MarqueeSelection et al are not exported

Some of the above should likely also be exported for customization to work as expected, but would like to double check.

Focus areas to test

Please ensure that the public API surface is as expected with these additional exports.

Microsoft Reviewers: Open in CodeFlow

@KevinTCoughlin
Copy link
Copy Markdown
Member Author

Hmm prettier seems to have opinions about AppState when I committed after resolving conflicts 0_0

@JasonGore
Copy link
Copy Markdown
Member

If you don't get a clean merge, prettier will run against all of the files that were merged in from master. :/ I usually get around it by reverting conflict files to allow an automerge then reapply. (We still need to figure out how so many files are changing on master without prettier running against them.)

I'm a bit concerned about exporting base implementations because I consider them somewhat of an component implementation detail. This will become more of an issue with createComponent. How exactly will they be used? I'm wondering if it would be better to require direct importing to not make it part of the default API surface.

@cliffkoh
Copy link
Copy Markdown
Contributor

cliffkoh commented Aug 6, 2018

The base components are viewed as the pure unstyled components, and the idea was always to allow styled variants to be created around these bases using styled. We are not fulfilling this promise ATM. Additionally, there is no way to completely opt out of Fabric styles if you want to have your own custom theme without the base component.

You could technically attempt to overwrite everything but think for a moment how unfeasible that is in reality :)

@JasonGore
Copy link
Copy Markdown
Member

This will get a bit more complicated if we roll out createComponent as there is no "base" component... although I guess we could create an equivalent without styling.

@cliffkoh
Copy link
Copy Markdown
Contributor

cliffkoh commented Aug 6, 2018

@KevinTCoughlin you need a change file.

@KevinTCoughlin
Copy link
Copy Markdown
Member Author

KevinTCoughlin commented Aug 10, 2018

@cliffkoh yea wasn't sure on where you both landed re: this change so didn't want to include a change to have it merged. Is this still worth doing with the createComponent work? We could keep it so they need to directly import the class, but it seemed worth doing to me giving the mergeStyles work for those that really expect the flexibility for a number of components. Also for consistency but that's less of a good reason.

If we're OK with this I'm assuming minor bump?

@KevinTCoughlin
Copy link
Copy Markdown
Member Author

KevinTCoughlin commented Aug 10, 2018

@cliffkoh + changefile w/ minor bump

@cliffkoh cliffkoh merged commit 248b46e into microsoft:master Aug 11, 2018
@KevinTCoughlin KevinTCoughlin deleted the keco/export-base-impls branch August 17, 2018 06:06
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

We do not uniformly export the "Base" version of components.

4 participants