Skip to content

Tile: exporting an enum.#4484

Merged
dzearing merged 4 commits intomicrosoft:masterfrom
Vitalius1:v-vibr/TileEnumExport
Apr 10, 2018
Merged

Tile: exporting an enum.#4484
dzearing merged 4 commits intomicrosoft:masterfrom
Vitalius1:v-vibr/TileEnumExport

Conversation

@Vitalius1
Copy link
Copy Markdown
Contributor

Pull request checklist

  • Addresses an existing issue: Fixes #0000
  • Include a change request file using $ npm run change

Description of changes

Exports an enum to use the values to create a PlaceholderTile component as an element in Shimmer component.
If values change in future the PlaceholderTile will be also using them.

Focus areas to test

(optional)

@Vitalius1 Vitalius1 requested review from ThomasMichon and atneik April 6, 2018 22:54
@Vitalius1 Vitalius1 changed the title Exports an enum from Tile.tsx needed for ShimmerTile creation. Tile: exporting an enum. Apr 6, 2018
}

const SIZES: {
export const SIZES: {
Copy link
Copy Markdown
Member

@dzearing dzearing Apr 9, 2018

Choose a reason for hiding this comment

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

instead of exporting "SIZES", can you rename this to something specific to tile, and use TitleCasing?

TileLayoutSizes would be reasonable.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Think of it like this: When you import {} from '@uifabric/experiments'; the intellisense will suggest importing SIZES. Does that make sense to the caller?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Got it. Thanks for the feedback. I guess if you are putting it in the context of intellisense when importing makes total sense.

@dzearing dzearing merged commit ec704f4 into microsoft:master Apr 10, 2018
@Vitalius1 Vitalius1 deleted the v-vibr/TileEnumExport branch April 10, 2018 01:40
@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 31, 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.

2 participants