Skip to content

Conversation

@Vitalius1
Copy link
Contributor

@Vitalius1 Vitalius1 commented Jul 13, 2018

Pull request checklist

  • Include a change request file using $ npm run change

Description of changes

Removing use of borderStyle prop of Shimmer subcomponents in favor of using mergeStyles API.

Microsoft Reviewers: Open in CodeFlow

@Vitalius1 Vitalius1 requested review from atneik and cliffkoh July 13, 2018 19:53
@cliffkoh
Copy link
Contributor

Sorry about the merge conflict. This fixes the copy of Shimmer in OUFR but not the one in experiments.. May I strongly recommend reconciling the fact that they're now forking? Either by removing the duplication or making the changes in both places.

Also, it might be good to add an example using the styles API to demonstrate how to customize the borderStyle (and to verify that it still works) :)

/**
* Sets custom styling of the gap.
*/
borderStyle?: IStyleSet;
Copy link
Contributor

Choose a reason for hiding this comment

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

@dzearing are you okay with removing this prop? It is technically a breaking change...

An alternative is just to change the type to any and add @deprecated. (Would be even nicer if we could use TS 3.0's unknown type instead for deprecation).

Copy link
Member

Choose a reason for hiding this comment

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

Unless it was a blocking bug, this should be @deprecated and not removed.

@cliffkoh cliffkoh requested a review from dzearing July 17, 2018 01:08
@cliffkoh
Copy link
Contributor

Request review and input from @dzearing as well.

@Vitalius1
Copy link
Contributor Author

@cliffkoh I will wait on you PR #5609 to get merged as it will cause some conflicts again.
@dzearing need your opinion on removing the prop. Technically is a breaking change but this API is not exposed anywhere in the documentation or examples so I feel like removing it in 99.9% should be safe.

Also I can add to this PR a re-export of Shimmer from experiments package pointing to the one from OUFR and remove the code. The caveat is that this would be a breaking change for anyone who used Shimmer from V6 of the experiments package as it uses a prop removed from the one in OUFR. Let me know how to proceed?

@dzearing dzearing self-assigned this Jul 27, 2018
@dzearing
Copy link
Member

@Vitalius1 can you clean up conflicts and deprecate the prop?

@Vitalius1 Vitalius1 requested a review from micahgodbolt as a code owner July 31, 2018 06:01
);
const showTooltip = isTooltipVisible && isContentPresent;
const ariaDescribedBy = (setAriaDescribedBy && isTooltipVisible && isContentPresent) ? tooltipId : undefined;
const ariaDescribedBy = setAriaDescribedBy && isTooltipVisible && isContentPresent ? tooltipId : undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line was changed by the auto formatter (prettier) after merging a fresh master and resolving the conflicts.

@Vitalius1
Copy link
Contributor Author

@cliffkoh @dzearing
Closing this PR due to some snapshots issues I had after some failed git manipulation 😢
Opening this one instead on a clean branch: PR 5559.

@Vitalius1 Vitalius1 closed this Jul 31, 2018
@Vitalius1 Vitalius1 deleted the v-vibr/ShimmerPropRemoval branch November 7, 2018 21:02
@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.

3 participants