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

Add prefix to CSS classes #349

Merged
merged 11 commits into from
Jan 6, 2021

Conversation

alex-lairan
Copy link
Contributor

@alex-lairan alex-lairan commented Dec 22, 2020

Description

This fix the possibility to have a conflict between external CSS and Mint CSS.
Most of external css are compiled and use letter as identifier (as mint does).

Skipping the 26 first reduce the chance that an important style apply.

It allows the user to specify a specific prefix for css classes.

  "application": {
    "css-prefix": "my_prefix-"
  },

@Namek
Copy link
Contributor

Namek commented Dec 22, 2020

I totally agree for the alternative design. This one is just a workaround. I have seen some CSS framework which had classnames like ee so this PR wouldn't fix that case. Do you think you'd be able to implement the alternative?

EDIT: I propose to make a configuration variable inside the mint.json instead of a parameter.

@alex-lairan
Copy link
Contributor Author

I may try, do you have any hint for using variables from the mint.json file ? :)

@Namek
Copy link
Contributor

Namek commented Dec 23, 2020

@gdotdesign can you advise?

@alex-lairan
Copy link
Contributor Author

I need to find a way to get values from mint.json and pass them into the src/style_builder.cr

This class shouldn't read the json directly because it creates an unnecessary coupling.

@gdotdesign
Copy link
Member

I like the idea of having a prefix for this, however my personal choice would be command line flag, but the application.css-prefix can work too.

Code wise we should can pass the it from the top level command to the compiler itself.

@alex-lairan
Copy link
Contributor Author

I don't know why, but on my personal app, I don't have any html, but from manual test, and from specs, it works.
¯_(ツ)_/¯

Can someone help me to find out why?

@alex-lairan alex-lairan changed the title Change initial css identifier from a to aa Add prefix to CSS classes Dec 23, 2020
core/tests/mint.json Outdated Show resolved Hide resolved
@alex-lairan
Copy link
Contributor Author

I'm confused because "css-prefix": "prefix-" cause a silent error that lead to an empty web page, but `"css-prefix": "prefix_" works perfectly well.

@gdotdesign
Copy link
Member

I'm confused because "css-prefix": "prefix-" cause a silent error that lead to an empty web page, but `"css-prefix": "prefix_" works perfectly well.

It's because the prefix contains a dash -. The selectors names are used to create the CSS variable map in the component which results in output like this:

$prefix-h() {
  const _ = {
    [`--a-a`]: `2px solid ` + this.aq,
    [`--a-b`]: this.ar
  };

  return _;
}

and this causes a JavaScript error.

What we need to do is to just use the un-prefixed version internally (in JavaScript) and use the prefixed one for the actual classes.

@gdotdesign gdotdesign requested a review from Sija December 28, 2020 10:41
@gdotdesign gdotdesign added the enhancement New feature or request label Dec 28, 2020
@gdotdesign gdotdesign added this to the 0.11.0 milestone Dec 28, 2020
spec/compilers_spec.cr Outdated Show resolved Hide resolved
src/builder.cr Outdated Show resolved Hide resolved
src/commands/compile.cr Outdated Show resolved Hide resolved
src/builder.cr Outdated Show resolved Hide resolved
src/compiler.cr Outdated Show resolved Hide resolved
src/compilers/top_level.cr Outdated Show resolved Hide resolved
src/mint_json.cr Outdated Show resolved Hide resolved
src/reactor.cr Outdated Show resolved Hide resolved
src/style_builder.cr Outdated Show resolved Hide resolved
src/style_builder.cr Outdated Show resolved Hide resolved
@gdotdesign gdotdesign merged commit b6f0dba into mint-lang:master Jan 6, 2021
@alex-lairan
Copy link
Contributor Author

🎉 Thanks for the fix !

gdotdesign added a commit that referenced this pull request Jan 6, 2021
Sija added a commit that referenced this pull request Jan 6, 2021
This was referenced Jan 6, 2021
Sija added a commit that referenced this pull request Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

4 participants