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

Changes css style specificity to 0 #753

Merged
merged 1 commit into from
Oct 9, 2017
Merged

Changes css style specificity to 0 #753

merged 1 commit into from
Oct 9, 2017

Conversation

majapw
Copy link
Collaborator

@majapw majapw commented Oct 6, 2017

While having specificity be quite high is a necessity in having react-with-styles-interface-css be fully operable, it is not actually necessary (with one small tweak) for react-dates itself. In order to have our exported css file be less of a trash-fire, I am reducing the specificity to zero.

to: @ljharb @lencioni

@majapw majapw added the semver-patch: fixes/refactors/etc Anything that's not major or minor. label Oct 6, 2017
Copy link
Contributor

@lencioni lencioni left a comment

Choose a reason for hiding this comment

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

I'm not really familiar with this registerMaxSpecificity function, so I'll have to leave the stamp up to someone else I think.

@@ -126,9 +126,9 @@ class CalendarDay extends React.Component {
modifiers.has('selected-start') ||
modifiers.has('selected-end');

const hoveredSpan =
const hoveredSpan = !selected && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this related?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes! So basically specificity is what generates something like:

.CalendarDay, 
.CalendarDay_1.CalendarDay_1,
.CalendarDay_2.CalendarDay_2.CalendarDay_2,

and then the appropriate class name is applied based on the position styles.CalendarDay takes in the css call.

In this particular method, we were relying on a particular order of hoveredSpan vs. selected to do styling overrides. By not applying the hoveredSpan class to selected days, we get around this necessity. As a result, this allows us to remove the specificity requirement and make the css file we ship with react-dates much smaller.

In general, we'd recommend using a high specificity value with RWS-ICSS (it defaults to 20), but in this small package, it's not strictly necessary.

Copy link
Member

Choose a reason for hiding this comment

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

So then this change means that selected days no longer have a hover effect?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is still a hover effect on selected days as defined by https://github.com/airbnb/react-dates/blob/4a8d379a2d23c1e0c265545b8ac7f2e78ab3a607/src/components/CalendarDay.jsx#L259-L263. The hovered span styling are completely different and before we were relying on the ordering of styles to apply selected over hovered. However, this change means that those styles will never be applied at the same time which will solve this issue.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.733% when pulling 4a8d379 on clean-up-exported-css into 32c536c on master.

@majapw majapw merged commit 9786c77 into master Oct 9, 2017
@majapw majapw deleted the clean-up-exported-css branch October 9, 2017 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch: fixes/refactors/etc Anything that's not major or minor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants