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

Added color and icon modules. #2537

Closed
wants to merge 25 commits into from

Conversation

ZeroCool940711
Copy link
Contributor

@ZeroCool940711 ZeroCool940711 commented Feb 12, 2024

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:

from nicegui import color, icon, ui

ui.button("NiceGUI", color=color.GREEN, icon=icon.THUMB_UP, on_click=lambda: ui.icon(icon.FACE))

ui.run(native=True, reload=True)

@rodja
Copy link
Member

rodja commented Feb 12, 2024

Thanks for the PR @ZeroCool940711! I feel that APP_ID and APP_ICON should not be located in icon.py but rather in app.native. They are only relevant when using Native Mode. And these additions should be a separate pull request because they are so very distinct from the color and icon auto-complete helpers you propose here.

@ZeroCool940711
Copy link
Contributor Author

Thanks for the PR @ZeroCool940711! I feel that APP_ID and APP_ICON should not be located in icon.py but rather in app.native. They are only relevant when using Native Mode. And these additions should be a separate pull request because they are so very distinct from the color and icon auto-complete helpers you propose here.

I understand. I will remove the APP_ID and APP_ICON code for now and see how I can implement them in the place you mentioned. Give me a minute to push a commit to correct that.

@ZeroCool940711
Copy link
Contributor Author

ZeroCool940711 commented Feb 12, 2024

@rodja Done. I pushed a commit to remove the APP_ID and APP_ICON from the icons.py. I've also updated the PR description to remove the information about the APP_ID and APP_ICON since its not needed.

@rodja rodja added the enhancement New feature or request label Feb 12, 2024
@rodja rodja requested a review from falkoschindler February 12, 2024 03:38
rodja
rodja previously approved these changes Feb 12, 2024
Copy link
Contributor

@falkoschindler falkoschindler left a 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:

  1. 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.
  2. Icons with suffixes like _SHARP don't seem to work, e.g. ui.icon(icon.YOUTUBE_SEARCHED_FOR_SHARP).
  3. 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 like white10.
  4. You don't cover Quasar and CSS colors, right? Why not?
  5. To make color and icon part of the public API, we should add it to nicegui/__init__.py.
  6. There's already an implementation in color_elements.py checking for Quasar and Tailwind colors. The latter is done using the BackgroundColor literal from the tailwind_types module. Maybe this can be useful for generating color.py, but I'm not sure.

@ZeroCool940711
Copy link
Contributor Author

  1. 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.

Both the colors.py and icons.py come from the Flet library, I just copied them from there.

  1. Icons with suffixes like _SHARP don't seem to work, e.g. ui.icon(icon.YOUTUBE_SEARCHED_FOR_SHARP).
  2. 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 like white10.
  3. You don't cover Quasar and CSS colors, right? Why not?

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.

  1. To make color and icon part of the public API, we should add it to nicegui/__init__.py.
  2. There's already an implementation in color_elements.py checking for Quasar and Tailwind colors. The latter is done using the BackgroundColor literal from the tailwind_types module. Maybe this can be useful for generating color.py, but I'm not sure.

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.

@ZeroCool940711
Copy link
Contributor Author

To make color and icon part of the public API, we should add it to nicegui/init.py.

I added both of them to the nicegui/__init__.py and will look into finding a way to automate adding new colors and icons, let me know what else you recommend I do.

@ZeroCool940711
Copy link
Contributor Author

ZeroCool940711 commented Feb 13, 2024

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 colors.py as well. So far with just the Tailwind colors we have 247 colors.

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_color.py script to the repo and also to the publish.yaml workflow since I think it should run there but let me know if I should move it somewhere else or you want to modify the workflow yourself later. I will go now and start working on the icons part but I think I can copy/paste most of the code from the colors and just change one or two things so it should be faster than doing everything from scratch.

Update 4: I added a script for the icons, we now get all the Google icons and they are added to the nicegui/icon.py. I also tried to get the Boostrap icons but seems like they don't work in NiceGUI properly or at all so, for now I left the code in the script ni case support for the Boostrap icons is needed, it will be as simple as removing two comments from the script. Last but not least, I added the update_icons.py script to the publish.yml as well so it can keep the icons up-to-date.

@ZeroCool940711
Copy link
Contributor Author

ZeroCool940711 commented Feb 13, 2024

Sorry about the forced pushes, I was fixing the commit message on the publish.yml to reflect the new files and had a few typos that I noticed only after pushing and reading the Files Changed tab here, it happened a few times 😓.

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.

Copy link
Contributor

@falkoschindler falkoschindler left a 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 and icon.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 like icon.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 like PRIMARY, INFO and so on. But where do colors like ICE_DUST come from?

  • Your basically re-implementing QUASAR_COLORS and TAILWIND_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 include SteelBlue, even though it is a valid color in NiceGUI.

  • I think colors like AMBER100 should map to amber-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 aliases color.tailwind and color.quasar.

@rodja
Copy link
Member

rodja commented Feb 20, 2024

I think it would be sufficient to stick with Quasar colors for auto-completion. Tailwind colors are accessable through .tailwind and css colors do not need auto-completion because they are in .style strings anyway.
This would remove much of the complexity and makes it simpler for the user to pick a color.

@falkoschindler
Copy link
Contributor

@rodja Regarding CSS colors I have to disagree. They are not only used in .style strings:

ui.icon('home', color='SteelBlue')

@ZeroCool940711
Copy link
Contributor Author

@rodja Regarding CSS colors I have to disagree. They are not only used in .style strings:

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.

@falkoschindler
Copy link
Contributor

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.

@ZeroCool940711
Copy link
Contributor Author

For CSS we could simply use webcolors. It's another dependency, but very lightweight. They define colors in ubernostrum/webcolors@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
Copy link
Contributor Author

I noticed while working on an IconPicker/IconInput element for working with the icons that many of the icons are not centered for some reason, I first thought that they were missing but it seems like they are just not centered on the element using them, if you place them in a button for example they can be outside the button container/area, if they overlap with another button or element they seem to interact with that element when you click in them, like, you click in one button and it interacts with the other button as well because they overlap, I think that's weird and probably a bug somewhere.
image
image
In the first screenshot above, I had my mouse over the button with the red line under it but the one next to it was the one that was being selected and showing the tooltip for, the second image shows the actual button tooltip if I hover in the top right corner of it almost leaving the button area.

@rodja
Copy link
Member

rodja commented Feb 22, 2024

@ZeroCool940711 could you share your test code?

@ZeroCool940711
Copy link
Contributor Author

@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,
    )

@rodja
Copy link
Member

rodja commented Feb 23, 2024

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

@rodja
Copy link
Member

rodja commented Feb 23, 2024

It looks like the icon name action_key is simply not supported. Let's have a closer look at the json fetched from the google fonts api:

{
      "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 update_icons.py script needs to evaluate the unsupported_families list.

@ZeroCool940711
Copy link
Contributor Author

I think the update_icons.py script needs to evaluate the unsupported_families list.

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 update_icons.py works with NiceGUI?

…arp icon types from both Material Icons and Material Symbols.
@ZeroCool940711
Copy link
Contributor Author

ZeroCool940711 commented Feb 23, 2024

@rodja I've improved the update_icons.py script to take into account the unsupported icons families, I would appreciate it if you could take a look at it and give some feedback on it. I added support for all the icons we were ignoring before like outlined, round/rounded and sharp, most of them seem to work okay now but there are a couple that are still broken, I can confirm that it is related to them not being supported but I'm not sure why the code I added for that is ignoring some icons and still adding them, also, the reason why the icons seem to not be centered is because when an icon is not supported the extra text added in front of it is being rendered in front of the actual icon and only a part of the string is taken into account by NiceGUI, so, it pushes the icon to the side. The list of broken icons is way less than before even tho I pretty much increased the number of icons we have by 4-7 times, we have about 21K icons now.

@rodja rodja dismissed their stale review February 23, 2024 15:10

Still a lot of changes...

This was referenced Feb 25, 2024
@falkoschindler
Copy link
Contributor

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.

@ZeroCool940711
Copy link
Contributor Author

@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 _ to missing and broken icons as well as duplicates, I mean, even the tests are failing on your PR, and you improved nothing on it other than having a PR to your name with your own commit history instead of the original, that's the big difference? WoW.

@falkoschindler
Copy link
Contributor

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:

  • Which icons are broken or missing?
  • Where do you see duplicates?
  • The tests are a bit flaky. There's only one failure out of >1500 tests which is unrelated to the icon module.
  • Yes, I argued against the leading underscore. But all other attempts seem too unintuitive, so that's my favorite compromise at the moment.

@falkoschindler
Copy link
Contributor

I'll close this stale PR. We will continue working on icons and colors on PRs #2613 and #2614 for version 2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants