- Notifications
You must be signed in to change notification settings - Fork22
Make icon size the same asQtViewerPushButton
for UI consistency.#103
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Make icon size the same asQtViewerPushButton
for UI consistency.#103
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Introduce a dependency on tinycss2. Then _use_ it via some convenienceparsing finding functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I like this approach more than regex, left some comments. Major request for change is to add a test for the fallback values, but implementation looks 👍
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
It's not a generic CSS parser but rather Napari-specific.Co-authored-by: David Stansby <dstansby@gmail.com>
Uh oh!
There was an error while loading.Please reload this page.
Solves#46 (with rather a lot of code).
Summary
Introduce a dependency ontinycss2. Thenuse it via some ...err... "convenience" parsing functions. Hopefully, these are clear and somewhat reusable for our eventual solution of#86.
Testing
Added a unit test for my function. Open to adding test cases if anyone can think of anything.
Reviewer notes:
The other ways to solve#46 is via a regex (~5 LoC) or via
thing.setIconSize(QSize(28, 28))
(2 LoC). Compare those with this @ ~30 LoC. 🙃Argument for doing it this way
Boils down to#86. We should be able to parse other colours from the "current" CSS file with these functions. Then do an internal update of the axes colours etc (currently most of the code in
NapariMPLWidget.apply_napari_colorscheme
). And Ithink our update code could be connected toviewer.events.theme
(undocumented... we can probably test quite easily and fix the docs upstream either way).Against
I'm actually increasingly open to a regex solution.It's what napari themselves seem to do. Opinions gratefully received.
Could also reuse the tdd and do the regex in
from_css_get_the_size_of
.Also if anyone knows of a better way to customise my subclass's constructor whilst keeping the type checker happy. Like maybe there's a better design decision to be made somewhere? Discussion of interest...