Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using composition to add more specificity to avoid style override #7171

Merged
merged 2 commits into from
Nov 20, 2018

Conversation

arkogupta
Copy link
Contributor

@arkogupta arkogupta commented Nov 20, 2018

Pull request checklist

Description of changes

The style applied to the borderWhileDragging class was getting overridden inconsistently by the style applied to the root class. So I used composition by making use of selectors to add more specificity to prevent it from getting overridden.

This demo link can be used to check that the border is indeed getting added and the selectors are getting applied:
https://msft.spoppe.com/teams/SPGroups/playground/cookieredirect/redirect.aspx?mobile=0&setcookie=https%3A%2F%2Fresourceseng.blob.core.windows.net%2Ffiles%2Fdev-arkgupta-arko-p510e
--git-odsp-next-app-min-master%2F&redirect=https%3A%2F%2Fmsft.spoppe.com%2Fteams%2FSPGroups%2FVNextDocLib%2FForms%2FAllItems.aspx
.

Here's what was happening before :

image

After the changes :

image

Focus areas to test

(optional)

Microsoft Reviewers: Open in CodeFlow

Copy link
Contributor

@aneeshack4 aneeshack4 left a comment

Choose a reason for hiding this comment

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

lgtm! Thank you for making this change.

@aneeshack4 aneeshack4 merged commit 92ba832 into microsoft:master Nov 20, 2018
@msft-github-bot
Copy link
Contributor

🎉office-ui-fabric-react@v6.106.2 has been released which incorporates this pull request.:tada:

Handy Links:

@aneeshack4
Copy link
Contributor

Hi @arkogupta we're rethinking this change. It looked harmless at first to me which is why I approved it but after speaking with my teammates, I realize that this change might cause problems in the future. OUIFR (office-ui-fabric-react) has a lot of different consumers and increasing the specificity of a selector to solve for one use case may cause issues for others.

There must be an underlying root cause that's causing this specificity issue in the first place. In traditional css, the specificity rule is simple - whichever rule's first defined in the document has higher specificity. However, mergeStyles mixes the classes to create one classname. So it's more likely that this mixing isn't being done properly and multiple classnames are being appended to one element causing "specificity wars". Adding selectors seems hacky and we'd like to avoid that. Can you please provide us a better solution? If you could first find the underlying mixing problem and fix that, that would be much appreciated. 😄

@arkogupta
Copy link
Contributor Author

arkogupta commented Nov 22, 2018

Hi @arkogupta we're rethinking this change. It looked harmless at first to me which is why I approved it but after speaking with my teammates, I realize that this change might cause problems in the future. OUIFR (office-ui-fabric-react) has a lot of different consumers and increasing the specificity of a selector to solve for one use case may cause issues for others.

There must be an underlying root cause that's causing this specificity issue in the first place. In traditional css, the specificity rule is simple - whichever rule's first defined in the document has higher specificity. However, mergeStyles mixes the classes to create one classname. So it's more likely that this mixing isn't being done properly and multiple classnames are being appended to one element causing "specificity wars". Adding selectors seems hacky and we'd like to avoid that. Can you please provide us a better solution? If you could first find the underlying mixing problem and fix that, that would be much appreciated. 😄

Makes sense @aneeshack4 . I have raised another PR addressing this( #7211 ). Please take a look at it.
Thanks.

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Border CSS getting overridden by consumer
3 participants