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: Add CSS variables for better organization and flexibility #1001

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

pkvach
Copy link
Contributor

@pkvach pkvach commented Mar 21, 2024

Checklist

  • All new and existing tests are passing
  • I have added an entry to CHANGES.rst because this is a user-facing change or an important bugfix
  • I have written proper commit message(s)

What changes does this Pull Request introduce?

Added CSS variables to improve theme customization and organization in isso.css.

Why is this necessary?

Closes #655

@pkvach pkvach force-pushed the feat/add-css-vars branch from 80a675d to 86243b8 Compare March 30, 2024 23:10
@pkvach pkvach force-pushed the feat/add-css-vars branch from 86243b8 to 762b300 Compare April 14, 2024 21:22
@jelmer jelmer merged commit d3d8ae5 into isso-comments:master Apr 18, 2024
15 checks passed
@pkvach pkvach deleted the feat/add-css-vars branch April 19, 2024 04:45
@ix5
Copy link
Member

ix5 commented Apr 19, 2024

Nice work, very cleanly done. LGTM as well.


One thing though: I'll reiterate what I wrote in

I do not have a particularly strong opinion on this either way. But I agree with @ vincentbernat in testing before blindly rewriting everything.

As a data point: https://caniuse.com/css-variables

Browsers from 2017 (Edge) might not support it. IE 10 doesn't support it. Many big-C Corporate/Enterprise people are still on LTS versions, one way or another. Also, Android 4.4 devices are still working perfectly fine.
Also, switching the CSS would break existing setups with custom CSS, so it'd require a version bump and a notice to users.

This is potentially a breaking change as it drops support for IE10 (not sure whether we were compatible before, but now we definitely aren't). We should communicate that somewhere (changelog) and also drop the claim from the homepage (docs/index.html) about IE10 compatibility.

@pkvach
Copy link
Contributor Author

pkvach commented Apr 19, 2024

Thank you for your attention to this matter.

I understand that there may be differing opinions regarding whether to continue supporting a browser that is still in use but no longer receives updates from its developer, leaving it vulnerable to security risks.

If needed, I can help with restoring support for IE 10 after this PR change.

@ix5
Copy link
Member

ix5 commented Apr 23, 2024

Thank you for your attention to this matter.

I understand that there may be differing opinions regarding whether to continue supporting a browser that is still in use but no longer receives updates from its developer, leaving it vulnerable to security risks.

If needed, I can help with restoring support for IE 10 after this PR change.

I might have miscommunicated here, IE10 support is not that important to me, but rather our claims to be compatible when we aren't anymore.

@pkvach
Copy link
Contributor Author

pkvach commented Apr 24, 2024

Great, thanks for clarifying.
Should I correct the documentation where IE10 support is listed?
And maybe add a note to the CHANGES.rst file.

@ix5 ix5 mentioned this pull request Apr 29, 2024
5 tasks
@ix5
Copy link
Member

ix5 commented Apr 29, 2024

Great, thanks for clarifying. Should I correct the documentation where IE10 support is listed? And maybe add a note to the CHANGES.rst file.

Already done #1022

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.

Use CSS vars
3 participants