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

Allow disabling of !important appending via aphrodite/no-important #104

Merged
merged 6 commits into from
Jul 20, 2016

Conversation

jlfwong
Copy link
Collaborator

@jlfwong jlfwong commented Jul 20, 2016

This is a non-breaking change to make it possible to use Aphrodite without it automatically appending !important to your styles, but does not make it the default.

Fixes #25.

Also see discussion in #102, #41.

@jlfwong
Copy link
Collaborator Author

jlfwong commented Jul 20, 2016

I made it aphrodite/lib/without-bang-important because I couldn't figure out how to make it aphrodite/without-bang-important, but I'm sure it's possible, so would appreciate some guidance.

Please refrain in this PR from discussing whether or not you think !important should be default behaviour or not, unless you are opposed to having this as an incremental step at all. This does not preclude the possibility of changing the default behaviour in the future.

@sophiebits
Copy link

You need a file in the root of the built package, which with the current setup probably means the root of the repo. It can be as simple as

module.exports = require('./lib/without-bang-important');

@jlfwong jlfwong changed the title Allow disabling of !important appending via aphrodite/lib/without-bang-important Allow disabling of !important appending via aphrodite/without-bang-important Jul 20, 2016

const css = (...styleDefinitions) => {
// Do not append !important to style definitions
return injectAndGetClassName(false, styleDefinitions);
Copy link
Contributor

Choose a reason for hiding this comment

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

To make it so you don't need the comment above, I would recommend giving that boolean a name:

const useImportant = false;
return injectAndGetClassName(useImportant, styleDefinitions);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see your intent, but I think the comment is more descriptive than that variable name, and it ends up being the same # of lines. If this thing had many arguments of ambiguous meaning, I'd be more inclined to the go the variable naming route (possibly still keeping the comments above the variable declarations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just prefer to avoid comments where I can because they can go out of date. Also, unless you know the function signature, you don't know that this comment is referring to the false argument... It almost looks like a todo comment that's missing the TODO pragma.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In personal experience, comments only tend to get out of date when they're far removed from the thing they apply to spacially (e.g. a comment discussion caveats of function use). I've rarely seen comments a single line above what they apply to get out of date when you have a sane code reviewing process in place.

I do see your point about the non-obvious connection between the comment and the false here, so I'm happy to take the hybrid approach and include the variable too.

@kentcdodds
Copy link
Contributor

This looks great to me

* Inject styles associated with the passed style definition objects, and return
* an associated CSS class name.
*
* @param {boolean} useImportant If true, will
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops, forgot the rest of this sentence. Will fix.

@sophiebits
Copy link

nit: a vote from me for aphrodite/no-important for brevity; I think it's about as clear.

validDefinitions.map(d => d._definition));

return className;
const useImportant = true; // Append !important to all style definitions
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, this is much more clear 👍

@JedWatson
Copy link

a vote from me for aphrodite/no-important for brevity; I think it's about as clear.

+1 from me too. I've never heard ! called "bang"s before now, they're just "exclamation marks". I read !important as "exclamation-important", so I suspect "no important" is clearer internationally.

Out of interest, I looked it up

This punctuation mark is called, in the printing world, a screamer, a gasper, a slammer, or a startler. In hacker culture, the exclamation mark is called "bang", "shriek", or, in the British slang known as Commonwealth Hackish, "pling".

@jlfwong jlfwong changed the title Allow disabling of !important appending via aphrodite/without-bang-important Allow disabling of !important appending via aphrodite/no-important Jul 20, 2016
@jlfwong
Copy link
Collaborator Author

jlfwong commented Jul 20, 2016

Switched to no-important, thanks for the feedback, @spicyj and @JedWatson!

@xymostech
Copy link
Contributor

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants