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

TailwindCSS class manager implementation #362

Merged
merged 19 commits into from
Apr 3, 2023
Merged

TailwindCSS class manager implementation #362

merged 19 commits into from
Apr 3, 2023

Conversation

Diegiwg
Copy link
Contributor

@Diegiwg Diegiwg commented Feb 10, 2023

PR in response to Issue #117, as a possible solution.

  • As requested , I uploaded the auxiliary lib project to the project's main.

  • Now, I hope you implement it in NiceGUI, so we can all enjoy a more practical way of styling.

- As requested, I uploaded the auxiliary lib project to the project's main.

- Now, I hope you implement it in NiceGUI, so we can all enjoy a more practical way of styling.
Copy link
Contributor Author

@Diegiwg Diegiwg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed my files, and I hope that you will continue the work I have done.

@falkoschindler falkoschindler changed the title (feature) TailwindCSS class manager implementation TailwindCSS class manager implementation Feb 10, 2023
@falkoschindler
Copy link
Contributor

Awesome, thanks @Diegiwg!

I started to refactor the code (my preferred way of getting to know new code). Now I'm at the point where we should decide what the final API should look like.

Currently it's like:

Typography(ui.label('label 2')).text_color('text-orange-900').element.tooltip('label 2')

Embracing the builder pattern we would certainly make Typography a member of the element:

ui.label('label 2').typography.text_color('text-orange-900').element.tooltip('label 2')

In order not to spoil the element's namespace too much, we could nest it within a tailwind field (or tw?). That empasizes the fact pretty well, that we're "only" manipulating Tailwind classes:

ui.label('label 2').tailwind.typography.text_color('text-orange-900').element.tooltip('label 2')
ui.label('label 2').tw.typography.text_color('text-orange-900').element.tooltip('label 2')

Furthermore, I tend to having methods per section, e.g. typography(), with multiple keyword arguments:

ui.label('label 2').tailwind.typography(text_color='text-orange-900').element.tooltip('label 2')

Another decision to make is what exactly we want to do with the "text-" prefix in this example. A possible rule might be to always remove the common prefix among all options for a certain property. For font-size this would be "text-", for background clip this is "bg-clip-" and so on.

@rodja What do you think?

@Diegiwg
Copy link
Contributor Author

Diegiwg commented Feb 10, 2023

Let's go:

  1. The idea of ​​having a 'tailwind' or 'tw' property is interesting, as it will expose the end user that the next functions applied are related to 'tailwind'.

  2. About passing the values ​​to 'typography', it's not a pattern that I like, I prefer to chain the methods.

  3. Removing common prefixes is essential, to decrease the length of arguments.

@morningstarsabrina
Copy link

I'm very excited for this change. thanks for your efforts.
for the implementation I prefer this solution

ui.label('label 2').typography(text_color='text-orange-900', font_size = "text-6xl").element.tooltip('label 2')

reasons:

  1. remove tailwind because at this point the maybe you can mix tailwind and CSS to add more customization in future and its shorter
  2. pass multi arguments to typography because its shorter and so you can know what typography offer as options
    thanks

@falkoschindler
Copy link
Contributor

Thanks, @Diegiwg and @morningstarsabrina, for your feedback and ideas!

We brainstormed different options once again:

  1. Builder pattern with namespace per section and methods per Tailwind property:

    ui.label().typography.text_color('orange-900').text_weight('bold').layout.width('full')
  2. Additional Tailwind namespace:

    ui.label().tailwind.typography.text_color('orange-900').text_weight('bold').layout.width('full')
  3. Single method per section with multiple keyword arguments:

    ui.label().tailwind.typography(text_color='orange-900', text_weight='bold').layout(width='full')
  4. Regular arguments instead of keyword arguments:

    ui.label().tailwind.typography('orange-900', 'bold').width('full')
  5. Methods per property without section namespaces:

    ui.label().tailwind.text_color('orange-900').text_weight('bold').width('full')
  6. Single method for all Tailwind properties (like current .classes() but with auto-suggestion for Tailwind):

    ui.label().tailwind('text-orange-900', 'text-bold', 'w-full')

One question is: How much value is in dividing properties into sections? If I'm looking for "flex grow", should I step into .layout? No, it's in .flexbox_grid (or whatever the name will be). Therefore we experimented with removing the section.

Once we are there, we could as well throw all possible values into a single .tailwind() method. This would, however, make "flex grow" options hardly discoverable. (Who knows that "grow" and "grow-0" are possible values for it?)

And - at least in VSCode - the suggestion popup for multiple keyword arguments gets cluttered quickly for many parameters. An alphabetically sorted dropdown list for class methods seems to be a nicer alternative.

So my personal tendency would be to use the Tailwind namespace, remove the sections, and use methods instead of keyword arguments. This would be proposal 5 in the list above. Oh, 5 and 6 could even be combined, so that tailwind can be called and stepped into. We could also allow it to accept a space-separated string like .classes for when you want to save space and don't need auto-completion.

@Diegiwg
Copy link
Contributor Author

Diegiwg commented Feb 16, 2023

After seeing all the options listed like this, I was tempted to choose option 5 as well.

About the option of a string for .tailwind, it might be interesting, but it wouldn't be a priority.

So I VOTE for option 5, with the tailwind namespace being responsible for all style methods.

@falkoschindler
Copy link
Contributor

falkoschindler commented Feb 18, 2023

Ok, I experimented with option 5/6 and it feels quite nice. I am, however, not sure if auto-suggestions for the 17,000 lines of generated code are fast enough. Sometimes it takes awfully long, but it might just need some time for indexing.

Here is a usage example:

from nicegui import ui
from nicegui.tailwind import Tailwind

ui.label('Test 1').tailwind.font_weight('extrabold').text_color('blue-600').background_color('orange-300')

title_style = Tailwind().font_size('2xl').font_weight('bold')

label2 = ui.label('Test 2')
label3 = ui.label('Test 3')

title_style.apply(label2)
title_style.apply(label3)

ui.run()

@Diegiwg
Copy link
Contributor Author

Diegiwg commented Feb 19, 2023

It's very good, for me, we use this model. About the thousands of rows, I believe the IDES will be smart to index after the first contact.

@morningstarsabrina
Copy link

wow can't wait to try it.
thanks for the hard work

@falkoschindler falkoschindler self-assigned this Mar 2, 2023
@falkoschindler falkoschindler added this to the v1.1.11 milestone Mar 7, 2023
@falkoschindler falkoschindler added the enhancement New feature or request label Mar 7, 2023
@rodja
Copy link
Member

rodja commented Mar 7, 2023

We want to merge this for the 1.1.11 release. As open ToDos we see

  • documentation
  • tests

@falkoschindler falkoschindler marked this pull request as draft March 7, 2023 13:27
@rodja rodja modified the milestones: v1.1.11, 1.1.12 Mar 13, 2023
@ofenbach
Copy link

What about this solution?

ui.typography('Test 1').font_size('2xl').font_weight('bold')

instead of

ui.label('Test 2').tailwind.font_weight('extrabold').text_color('blue-600').background_color('orange-300')

Typography suggests that we use styling for a text, maybe even pre defined styleguides.
This is usually the case, for example like this:

ui.typography(text="text", variant="h1")

Typography as a major element makes more sense to me since it is very common among frameworks/design systems.

Feel free to disagree :)

@falkoschindler falkoschindler modified the milestones: v1.2.1, v1.2.2 Mar 21, 2023
@falkoschindler
Copy link
Contributor

Thanks for the suggestion, @ofenbach!

We would rather not introduce a new UI element for this purpose, since it only solves a subset of Tailwind styling and there's no reason a ui.label shouldn't understand typography commands. But it's certainly a valid approach for custom design systems like CatDesign. 😀

@falkoschindler falkoschindler modified the milestones: v1.2.2, v1.2.3 Mar 24, 2023
@rodja rodja modified the milestones: v1.2.3, v1.2.4 Mar 30, 2023
@falkoschindler
Copy link
Contributor

I think this PR is basically ready to merge. Documentation and tests are done. The last thing I did was to split the 20,000 lines into separate files which improves the auto-completion performance quite a bit.

And I removed the Literal type for calling .tailwind(...) with arbitrary Tailwind classes. This list of strings was so huge (about 10,000 entries) that it slowed down everything, even just duplicating a line of code and waiting for the syntax highlighting to re-appear. Now calling .tailwind(...) simply expects strings. If you want to discover classes, you can use the individual methods.

@falkoschindler falkoschindler marked this pull request as ready for review March 31, 2023 16:11
Copy link
Member

@rodja rodja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is soooo cool :-)

@rodja rodja merged commit d510058 into zauberzeug:main Apr 3, 2023
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.

5 participants