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

css, js: Prefix all directives with 'isso-' #816

Merged
merged 4 commits into from
Apr 14, 2022

Conversation

ix5
Copy link
Member

@ix5 ix5 commented Mar 20, 2022

This "namespaces" Isso's CSS in order not to conflict with things on page

Fixes #442

@ix5 ix5 added client (Javascript) client code and CSS feature labels Mar 20, 2022
@ix5 ix5 added this to the 0.13 milestone Mar 20, 2022
@ix5
Copy link
Member Author

ix5 commented Mar 21, 2022

cc @mycahp

@ix5
Copy link
Member Author

ix5 commented Mar 21, 2022

Also, while I have your attention: @Kerumen @adroste

@adroste
Copy link

adroste commented Mar 21, 2022

@ix5 This is definitely a breaking change regarding custom overrides. So anything below v0.13 could lead to mad users.

@ix5
Copy link
Member Author

ix5 commented Mar 21, 2022

Would you mind going over the change and testing whether I missed anything or fat-fingered some string?

@adroste
Copy link

adroste commented Mar 21, 2022

Looks good (without testing it locally).

At first I thought you missed the input form selectors, but they are limited by the postbox scope.

https://github.com/posativ/isso/blob/88653210660af2069329896f8fb05d603b9881f4/isso/js/app/isso.js#L88-L91

Nevertheless, you could also prefix the keys for the items that are persisted in the localstorage:

https://github.com/posativ/isso/blob/88653210660af2069329896f8fb05d603b9881f4/isso/js/app/isso.js#L122-L124

@ix5
Copy link
Member Author

ix5 commented Mar 21, 2022

Nevertheless, you could also prefix the keys for the items that are persisted in the localstorage:

https://github.com/posativ/isso/blob/88653210660af2069329896f8fb05d603b9881f4/isso/js/app/isso.js#L122-L124

That's a great catch, thank you! Moving to prefixed key names will ofc invalidate any username/email/website users have set, but imo it's not worth to write a migration for that.

(Note that I hope you just happened to review the version I pushed after I rebased it on #822 ;-)

ix5 added a commit to ix5/isso that referenced this pull request Mar 21, 2022
This way, they are not interfering with other elements on
the "host" page. Note that the LocalStorage is scoped inside
the page isso is embedded on, not its API endpoint.

Suggested by @adroste in isso-comments#816 (comment)
Thanks!
@Kerumen
Copy link

Kerumen commented Mar 23, 2022

Looks good! Without testing. But this will be a great improvement of the library :)

ix5 added 4 commits April 14, 2022 22:27
This "namespaces" Isso's CSS in order not to conflict with
things on page

Fixes isso-comments#442
Prepending `isso-` to the element classes causes a change in
the generated HTML and necessitates an update of the
snapshot.
This way, they are not interfering with other elements on
the "host" page. Note that the LocalStorage is scoped inside
the page isso is embedded on, not its API endpoint.

Suggested by @adroste in isso-comments#816 (comment)
Thanks!
@ix5 ix5 merged commit 76127ff into isso-comments:master Apr 14, 2022
@ix5 ix5 deleted the css-namespace branch April 14, 2022 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client (Javascript) client code and CSS feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

textarea class conflicts with CSS libraries
3 participants