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.

Sign up for GitHub

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

Jump to bottom

Removing inline styles #308

Merged
merged 24 commits into from
Nov 28, 2023
Merged

Removing inline styles #308

merged 24 commits into from
Nov 28, 2023

Conversation

marekdedic
Copy link
Contributor

@marekdedic marekdedic commented Nov 16, 2023 •
edited
Loading

I've made it so no inline styles will be produced, unless you add a <Fa style="..."> property - then you can't really be helped :D

Additionally, I've made it so the docs extract their CSS into a separate file, I mainly did it because I used the docs to test the thing (and you need CSS in <head> or a separate file to not trigger CSP), but it should improve performance, so I left it in the PR...

@marekdedic marekdedic changed the title Rremoving inline styles Removing inline styles Nov 16, 2023
@marekdedic
Setting line-height for large size in class instead of inline style
1c05927
@marekdedic
Moved size-lg to be global and shared between elements
5a2cf92
@marekdedic
Renamed global size-lg class
36786ec
@marekdedic
Setting vertical-align in class instead of inline style
0b39494
@marekdedic
Setting transform-origin in class instead of inline style
e2179eb
@marekdedic
Setting origin in class instead of inline style
06a6c56
@marekdedic
Added class for fixed-width
37d7abe
@marekdedic
fw styles in a class
6360433
@marekdedic
Setting height in class instead of inline style
7c11810
@marekdedic
Setting pull in class instead of inline style
80d85ed
@marekdedic
Setting icon sizes in class instead of inline style
41da178
@marekdedic
Replaced static inline styles with a class
44328da
@marekdedic
Setting custom sizes with direct JS element manipulation
c78684a
@marekdedic
Fixed use of undefined bound element in custom size setting
7cff0fa
@marekdedic
Setting text color with direct JS element manipulation
a65663c
@marekdedic
Setting text transform with direct JS element manipulation
5ec3506
@marekdedic
Removed obsolete joinCss function
fd59136
@marekdedic
Extracting styles in docs
acbdb08
@marekdedic
docs build artifact sync
ed2c8fe
@marekdedic
Not including style attribute without styles to not trigger CSP
9d896a9
@marekdedic
Updated tests to use computed styles
c54a43e
@marekdedic
Made toggled classes not global
d5ae23b
@marekdedic
Fixed test typo
d253597
@marekdedic
Removed docs htaccess, shouldn't have been commited in the first place
25866fb
@marekdedic marekdedic marked this pull request as ready for review November 26, 2023 12:50
@Cweili Cweili merged commit d91656d into Cweili:master Nov 28, 2023
3 checks passed
@Cweili
Copy link
Owner

Cweili commented Nov 28, 2023

Thanks!

@marekdedic marekdedic deleted the no-inline-styles branch November 29, 2023 14:38
@marekdedic
Copy link
Contributor Author

Hi, sorry to bother, but could you please release this change? Thanks!

@Cweili
Copy link
Owner

Cweili commented Dec 19, 2023

@marekdedic I apologize for taking so long to reply to you.

I noticed that making this change would cause the package to lose one of its features, "No CSS file required."

Can we use a solution similar to ".style.background = 'red';" to solve the CSP issue?

@Cweili Cweili mentioned this pull request Dec 19, 2023
Closed
@marekdedic
Copy link
Contributor Author

Hi,
what do you mean by that? You can still use this package without a separate CSS file (CSP won't work for you then, but that's kind of the point of it :D). I re-configured the docs to extract the CSS here, but the users don't have to do this (they can decide)...

@Cweili
Copy link
Owner

Cweili commented Jan 2, 2024 •
edited
Loading

The solution similar to https://stackoverflow.com/a/57633533. Which you mentioned in this comment, does not require extracting the CSS.
Extracting CSS files requires users to have additional build configurations to load these CSS files.

@marekdedic
Copy link
Contributor Author

The current solution also doesn't require extracting the CSS - it's up to the user. If they want to keep the CSS as part of the JS, they can (they won't be able to have strict CSP, but that's kind of the point of strict CSP...).

@Cweili
Copy link
Owner

Cweili commented Jan 5, 2024

Sure, I think we can release this together with this PR. It will be a BREAKING CHANGE.

@Cweili
Copy link
Owner

Cweili commented Jan 5, 2024

Released with v4. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@marekdedic @Cweili