-
-
Notifications
You must be signed in to change notification settings - Fork 645
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
Added color and icon modules. #2537
Conversation
Thanks for the PR @ZeroCool940711! I feel that |
I understand. I will remove the |
@rodja Done. I pushed a commit to remove the |
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.
Thanks for the pull request, @ZeroCool940711!
I have some concerns though:
- How did you generate these files? Since we need to maintain and update them whenever we include a new version of the icons font, we'd ideally have a generator script we can simply run again.
- Icons with suffixes like
_SHARP
don't seem to work, e.g.ui.icon(icon.YOUTUBE_SEARCHED_FOR_SHARP)
. - What is
color.ON_PRIMARY
supposed to be? I don't know color identifiers with such a prefix and they don't seem to be working. Same for shades of white likewhite10
. - You don't cover Quasar and CSS colors, right? Why not?
- To make
color
andicon
part of the public API, we should add it tonicegui/__init__.py
. - There's already an implementation in
color_elements.py
checking for Quasar and Tailwind colors. The latter is done using theBackgroundColor
literal from thetailwind_types
module. Maybe this can be useful for generatingcolor.py
, but I'm not sure.
Both the colors.py and icons.py come from the Flet library, I just copied them from there.
Some colors and icons might not work since they are directly taken from Flet and it is based on flutter icons and colors dart files. We should find a way to get these colors and icons automatically from the libraries NiceGUI uses and use CI to generate these files which is why I tried to keep them as individual files that can be modified without affecting the rest of the codebase, I mentioned that on the discord conversation but I forgot to mention that here on the PR.
I tried to avoid modifying anything other than the files I added since some tests are failing locally so I'm not able to make sure I don't break something else, even on a clean clone of the repo on my setup and I need to look into that first, I initially just shared both files on discord for those that wanted to use them as extra utils on their project but it was suggested that I should do a PR with them so I tried to find a way to incorporate them as part of NiceGUI itself which is what this PR is about. The added colors and icons works in the most part and it's meant to be improved later on and modified to match what NiceGUI needs, this is more like a proof of concept on an alternative way we could implement colors and icons and have them accessible for easy discovery by the IDE autocomplete, it is also meant to reduce the learning curve of using NiceGUI which is related to the discussion on #2538 . I will look into how we can get the colors and icons from Quasar, CSS, HTML and Tailwind and improve this PR, I'm open to suggestions on how to do that. |
I added both of them to the |
just a little update, I found this file from the Tailwind repo which we can use to get the colors for Tailwind and keep them up-to-date. I started to work on script that we can use. After Im done with tailwind I will move to the rest of the colors we need and see how many of them I can get. Update 2: I also found the Quasar file where we have all the colors and I am working now on adding them to the Update 3: I finished the script to get the colors for both Tailwind and Quasar colors, we have in total 558 colors, they all should work but let me know if any of them doesn't work for some reason. I added the Update 4: I added a script for the icons, we now get all the Google icons and they are added to the |
…ctive repository in order to keep them always up-to-date.
133013e
to
af93d72
Compare
… them up-to-date.
4601217
to
b30c550
Compare
b30c550
to
86af392
Compare
86af392
to
3041da4
Compare
Sorry about the forced pushes, I was fixing the commit message on the I think this PR is ready for review now since I think I have addressed all the issues it had. I still think we could improve it with more icons and colors but I think this is enough for now, we can always come back to this and add more stuff if needed. |
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.
Hi @ZeroCool940711, here are my thoughts after reviewing your updated pull request:
-
We should remove the GitHub workflow for now. Since
color.py
andicon.py
need to stay in sync with the Tailwind and Quasar versions managed via npm.py, we shouldn't pull new icons or colors if we don't update the libraries as well. And because updates could cause breaking changes, we only do that manually before major releases. -
There are many manually managed icons in
number_words
. I'm not quite sure what to do with them, but the list of more than 100 special cases seems rather fragile. A_
prefix could be a generic alternative, but could mislead linters because it usually denotes private attributes. -
There are many duplicate icons. This is probably related to my next point.
-
What about different families (outlined, symbols, ...)? How could an API look like for different families?
icon.sharp.HOME
,icon.SHARP_HOME
,icon.HOME_SHARP
, or something different? The default "filled" icon could still be accessed likeicon.HOME
. What do you think? We should probably agree on an API first before updating the generator script. -
You're downloading the icons via HTTP. We should switch to HTTPS.
-
There are many manually managed colors in
quasar_colors
. I understand colors likePRIMARY
,INFO
and so on. But where do colors likeICE_DUST
come from? -
Your basically re-implementing
QUASAR_COLORS
andTAILWIND_COLORS
from color_element.py. We should either use those directly or use the colors from color.py there. -
Can we add support for CSS colors? It seems kind of misleading if the
color
namespace doesn't includeSteelBlue
, even though it is a valid color in NiceGUI. -
I think colors like
AMBER100
should map toamber-100
, not the hex value#...
. Currently some of your colors map to class names, others to hex values, which makes them unusable in.style()
or.classes()
calls. -
Should we distinguish color groups? This might improve clarity and explains where the color names come from:
color.css.RED
,color.tw.RED_100
,color.q.RED_1
. Additionally we could provide aliasescolor.tailwind
andcolor.quasar
.
…cons update from the workflow.
I think it would be sufficient to stick with Quasar colors for auto-completion. Tailwind colors are accessable through |
@rodja Regarding CSS colors I have to disagree. They are not only used in ui.icon('home', color='SteelBlue') |
That is one of the ways I mainly use the colors and icons introduced in this PR, in buttons and other elements that take a color or icon argument, it's much easier than the normal method, it is also, at least for me, to stick with a color scheme/palette. |
For CSS we could simply use webcolors. It's another dependency, but very lightweight. They define colors in https://github.com/ubernostrum/webcolors/blob/trunk/src/webcolors/constants.py based on current web standards. |
I added all the HTML4, CSS2, CSS21 and CSS3 colors from webcolors constants as well as webcolors as a dependency. Im not sure if I placed that dependency on the right place so, feel free to let me know where to put them if thats the wrong place. |
@ZeroCool940711 could you share your test code? |
@rodja sorry I didn't answer before, just got up from bed. I had just started making a prototype for the element I would be working on so, the code is pretty basic but it should show you the issue I was talking about: from nicegui import icon, ui
@ui.page("/")
def index():
with ui.card().style("max-width: 400px;"):
ui.select(["Google Icons"], value="Google Icons")
with ui.input(placeholder="Search icons..."):
ui.icon("search").style(
"position: absolute; right: 1rem; top: 50%; transform: translateY(-50%);"
)
with ui.row():
icon_vars = vars(icon)
icons = {k: icon_vars[k] for k in list(icon_vars.keys())[8:78]}
for icon_name, icon_value in icons.items():
ui.button(
icon=icon_value,
on_click=lambda e=icon_value: ui.clipboard.writeText(e),
).tooltip(icon_name)
if __name__ in {"__main__", "__mp_main__"}:
ui.run(
port=5000,
native=True,
reload=True,
dark=True,
) |
Well... my minimal reproduction is this: from nicegui import ui
ui.button(icon='action_key').classes('w-48')
ui.run() Maybe our font file is not correct? I've created a new issue: #2601 |
It looks like the icon name {
"name": "action_key",
"version": 251,
"popularity": 31,
"codepoint": 62722,
"unsupported_families": [
"Material Icons",
"Material Icons Outlined",
"Material Icons Round",
"Material Icons Sharp",
"Material Icons Two Tone"
],
"categories": [
"UI actions"
],
"tags": [
"buttons",
"circles",
"computer",
"device",
"dots",
"filter",
"find",
"glass",
"grid",
"hardware",
"input",
"keyboard",
"keypad",
"look",
"magnify",
"magnifying",
"search",
"see"
],
"sizes_px": [
20,
24,
40,
48
]
} I think the |
mmm, I'm not sure what to do, a lot of icons that are technically not supported do render properly and centered as they should. What do you recommend I do to make sure the icons list we get on |
…arp icon types from both Material Icons and Material Symbols.
@rodja I've improved the |
This PR is getting pretty big and complex. Instead of trying to review and improve the existing implementation, I created PR #2613 and PR #2614 as an alternative approach. They are heavily inspired by the discussion on this thread, but cut some corners here and there. By splitting the problem into separate PRs, I hope to simplify the discussions and improve focus on each subject. Tests and documentation are still missing though. |
@falkoschindler For the colors PR I think that one is actually good and that was one of the things I was trying to do to fix this PR later on but the other PR, from what I can see on your icon PR you just completely ignored all the talks we had here about not doing some stuff in some way because it would have negative effects in how people use things, and you also ignored all the fixes that I had to break my head to do for things to be at the point where I got this PR to, then you went with the most basic copy/paste implementation you could do in an hour, for which your icon implementation PR still has all the issues of the original PR that I later fixed, from variable names starting with |
Sorry, @ZeroCool940711, it wasn't my intention to just have a PR on my name. And I didn't copy&paste, but spent quite a while to re-implement these features. As mentioned on the PR for the icon module, I had something more elaborate in mind which didn't work. Regarding your points of criticism:
|
This PR adds color and icon modules for easy access to some colors and icons that can be found by the IDE autocomplete and code suggestions as mentioned in this discord conversation. These colors and icons can be used in the following way: