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

Replace display:none with .hidden #85

Conversation

craigloftus
Copy link
Contributor

The main reason I'm suggesting this change is to better facilitate using RecurrenceWidget with a style reset (e.g., cleanslatecss) to get around collisions with other stylesheets.

@dominicrodger
Copy link
Contributor

Seems reasonable to me - but I'm curious, what collisions would the current approach (i.e. effectively inline styles) cause?

@craigloftus
Copy link
Contributor Author

The style reset uses !important rules to set (among other things):

div {
  display: block !important;
}

Then to get the widget css to work again you set all those rules to also be !important to override the reset styles. I do that using less to process the original stylesheet:

#makeRecurrenceImportant() {
    @import (less) "recurrence.css";
}

& {
    #makeRecurrenceImportant() !important;
}

But that does not effect any inline styles.

Also, everyone knows classes are better than inline styles ;)

@dominicrodger
Copy link
Contributor

That seems pretty crazy to me - doesn't that mean having !important all over the place?

Regardless - this seems like a reasonable change, and you are of course right that classes are better than inline styles, so I'm happy for this to go in.

@craigloftus
Copy link
Contributor Author

craigloftus commented Feb 6, 2017

It is slightly crazy, but the !important rules of the reset and widget are restricted by containing classes which I didn't include in the example for simplicity.

Unfortunately atm I am stuck between this crazy approach and re-implementing the recurrence widget... which seems ever so slightly more crazy for what is not a large project.

@emperorcezar emperorcezar added this to the 1.6 milestone Jul 18, 2017
@dominicrodger dominicrodger merged commit 03b31d8 into jazzband:master Nov 8, 2018
dominicrodger added a commit that referenced this pull request Nov 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants