Skip to content

ShimmerComponent new props#4254

Merged
dzearing merged 51 commits intomicrosoft:masterfrom
Vitalius1:v-vibr/Shimmer
Mar 20, 2018
Merged

ShimmerComponent new props#4254
dzearing merged 51 commits intomicrosoft:masterfrom
Vitalius1:v-vibr/Shimmer

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

Addressing the feedback from previous PR.
Adds new props and changes the name of other ones.
Changes in logic to prepare the component being used width DetailsList

Focus areas to test

Naming of the props if clear and general enough.

Vitalie Braga added 30 commits February 26, 2018 18:34
@Vitalius1 Vitalius1 changed the title V vibr/shimmer ShimmerComponent new props Mar 13, 2018
@@ -0,0 +1,11 @@
{
"changes": [
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.

@Vitalius1 are you working with someone on this? It would be great to have pair ownership to ensure we have someone who can more deeply provide feedback and shared fixing.

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.

Yes I am. Mainly I get feedback from @atneik .

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.

@atneik can you review please?

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.

@atneik can you review please?


export const enum ShimmerElementType {
RECTANGLE = 'line',
LINE = 'line',
Copy link
Copy Markdown
Member

@dzearing dzearing Mar 20, 2018

Choose a reason for hiding this comment

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

These enum values should just be camelCased.

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. Will definitely change them in my next PR. Thanks!

@dzearing
Copy link
Copy Markdown
Member

@Vitalius1 I left a comment about enum naming, but will still merge this. Maybe you could fix in your next PR :)

@dzearing dzearing merged commit afd92f7 into microsoft:master Mar 20, 2018
@Vitalius1 Vitalius1 deleted the v-vibr/Shimmer branch April 12, 2018 23:42
@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