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

Change to importable CSS #1607

Closed
NullVoxPopuli opened this issue Nov 2, 2023 · 11 comments
Closed

Change to importable CSS #1607

NullVoxPopuli opened this issue Nov 2, 2023 · 11 comments

Comments

@NullVoxPopuli
Copy link
Contributor

few reasons:

  • some folks may want their own CSS
  • some folks don't want preprocessors (less, scss, which are forced right now)
  • allows folks to opt in to which css customizations they want.

potential examples:

// app.js
import 'ember-power-select/styles';
import 'ember-power-select/theme/material'; // or whatever
@mkszepp
Copy link
Collaborator

mkszepp commented Jan 2, 2024

@NullVoxPopuli ember-basic-dropdown and ember-power-select are now V2 addon (both were released for now as beta v8.0.0-beta.0). The migration to v2 addon has bring also some braking changes see #1619 & cibernox/ember-basic-dropdown#728

The css is now not anymore auto-imported. It must be manually done like you have described.

CSS import example is present in documentation:

@johanrd
Copy link
Contributor

johanrd commented Jan 2, 2024

I also stumbled upon the change in v2 when importing css in vanilla css contexts. FWIW, I found it possible to import the css directly into /assets/styles.css like this:

@import 'ember-power-select/vendor/ember-power-select.css';

(It may be a matter of personal taste, but I sometimes find it cleaner to import css directly in css, and not via js.).

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Jan 2, 2024

Importing in css is fine, but more confusing to recommend, because in order to get the css to be part of your module graph, you need css imported into js.
That is, it's not clear if importing in to css is from special css that already participates in the module graph, or if from the legacy style system: which [the legacy system] is always global, and can't participate in route based (or any kind of) splitting ever

(Docs could always be sure to mention this, however)

@mkszepp
Copy link
Collaborator

mkszepp commented Jan 2, 2024

There are a lot of options to import the css in the app... but i think we should add only one way in documencation (which is already), because i think that is the recommended way.
Since the file is public everyone can add it like he want. We can block that only by creating a "-private" folder, but if somebody want to import from "-private" he has always the possibility.

Option 1 (recommended):

// app.js or other js file
import 'ember-power-select/styles';

Option 2:

/* assets/styles.css */
@import 'ember-power-select/vendor/ember-power-select.css';

Option 3:
removed

@NullVoxPopuli
Copy link
Contributor Author

Option 3 should be banned from historical records 😅

Option 2 in fine for tiny apps.

But only importing in js, or css-that-participates-in-the-module-graph, can you canditionally get the power-select css upon an await import(..) component/route/whatever.

but i think we should add only one way

This would be ideal, but (un)fortunately we don't have only one way.
Hopefully soon tho!

@mkszepp
Copy link
Collaborator

mkszepp commented Jan 2, 2024

okay thank you!
I have removed option 3 in my comment 😅 (before somebody use that :D)...

If i have understand correctly, we should add option 2 also in docs with example from a route and describe, that there is also possible to use in component and controllers... right?

The blueprint adds always this lines in app.js like in your initial comment

@NullVoxPopuli
Copy link
Contributor Author

If i have understand correctly, we should add option 2 also in docs with example from a route and describe, that there is also possible to use in component and controllers... right?

yeah, exactly.

The blueprint adds always this lines in app.js like in your initial comment

yeah, it's the lazy documentation way.

And depending on your app's build config, you can use

@import 'ember-power-select/styles`

and have it participate in the module graph.

It's a weird balance between trying to document what's possible, and trying not to cover all cases possible.
I like to lean towards over-documenting this sort of stuff, because build behavior is super opaque to folks unfamiliar with how crazy JS build tooling is.

@mkszepp
Copy link
Collaborator

mkszepp commented Jan 3, 2024

@NullVoxPopuli @johanrd i have added the option in docs... would you like to review before we merge it (see #1627)?

Docs preview: https://deploy-preview-1627--rubber-tapper-eddies-44787.netlify.app/docs/installation

@johanrd
Copy link
Contributor

johanrd commented Jan 3, 2024

oh, great. Looks good to me.

I learnt something new regarding the module-graph benefits of importing through js, thanks! Regarding the benefits – if i understand it correctly – importing directly to app.js, won't give any module loading benefits, right? – you would rather need to import 'ember-power-select/styles'; in all the actual components / controllers / routes that are using ember-power-select.

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Jan 3, 2024

Correct

@mkszepp looks good!

@mkszepp
Copy link
Collaborator

mkszepp commented Jan 3, 2024

Great! Many thanks for inputs... PR is merged and so i think, that we can also close this issue

@mkszepp mkszepp closed this as completed Jan 3, 2024
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

No branches or pull requests

3 participants