Skip to content

Shimmer: fixing casing on enums#4542

Merged
manishgarg1 merged 6 commits intomicrosoft:masterfrom
Vitalius1:v-vibr/EnumFix
Apr 13, 2018
Merged

Shimmer: fixing casing on enums#4542
manishgarg1 merged 6 commits intomicrosoft:masterfrom
Vitalius1:v-vibr/EnumFix

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

Fixing casing on enums.

@Vitalius1 Vitalius1 requested review from atneik and dzearing April 13, 2018 02:02
@Vitalius1 Vitalius1 changed the title V vibr/enum fix Shimmer: fixing casing on enums Apr 13, 2018
}

export const enum ShimmerElementVerticalAlign {
CENTER = 'center',
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

CENTER [](start = 2, length = 6)

I am a bit confused. Why would we have two groups of enums ?

Copy link
Copy Markdown
Contributor Author

@Vitalius1 Vitalius1 Apr 13, 2018

Choose a reason for hiding this comment

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

@manishgarg1 The old ones are left until changes will be released and experiments package version will get bumped in odsp-common where ShimmerElementType.LINE is used in FileHoverCard. When that will be updated I will remove them from here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Got it. In that case I would recommend putting a comment like this. "@deprecated - The CAPS lock values will be deprecated soon"

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.

I thought about doing this, but I don't know if it makes sense as I know the only place outside the Fabric it has been used where it could break the contract with the component.
The component is still in experiments package and is if somebody put it into production code they were warned that it might brake due to API changes. So because I know were it is used in odsp-common I will just replace them in there and right after will remove them in here.

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.

Where would be the right place to put @deprecated?

@manishgarg1 manishgarg1 self-assigned this Apr 13, 2018
@Vitalius1
Copy link
Copy Markdown
Contributor Author

@dzearing Good to merge?

@manishgarg1 manishgarg1 merged commit a1d92a6 into microsoft:master Apr 13, 2018
@manishgarg1
Copy link
Copy Markdown
Collaborator

Looks straightforward to me hence merging.

@Vitalius1 Vitalius1 deleted the v-vibr/EnumFix branch April 16, 2018 01:24
@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