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

Update tailwind_field.py #163

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Conversation

Thutmose3
Copy link
Contributor

No description provided.

@Thutmose3
Copy link
Contributor Author

@GitRon could you check this one out?

@GitRon
Copy link
Contributor

GitRon commented Feb 13, 2024

Hi @Thutmose3!

Interesting idea! But "docs or it didnt happen" - so we'd need some way to tell the users how they can use this.

@smithdc1 @carltongibson What do you think about this approach? I'm still the new kid on the block 🙃

@Thutmose3
Copy link
Contributor Author

Ok, i'm adding a few more improvements, and will document it

@blasferna
Copy link
Contributor

blasferna commented Feb 13, 2024

Hi @Thutmose3,

I just wanted to comment that this package supports class customization by widget as follows:

settings.py

CRISPY_CLASS_CONVERTERS = {
    'textinput': 'dark:text-gray-100 dark:bh-gray-700'
}

It's just not documented.

Does this new functionality conflict with CRISPY_CLASS_CONVERTERS?

Regards.

@GitRon
Copy link
Contributor

GitRon commented Feb 13, 2024

Oh, nice one @blasferna! Do you think you have the time to add a small snippet about it to the docs?

@blasferna
Copy link
Contributor

Hello @GitRon,

CRISPY_CLASS_CONVERTERS apparently adds the classes defined in settings.py to the widgets rather than replacing them. In many scenarios, replacement would be required.

I could add some examples to the documentation without any problem.

@Thutmose3
Copy link
Contributor Author

Credit for this PR goes to @amitjindal as i basically copied his proposals from here: #90.

This feature should not conflict with CRISPY_CLASS_CONVERTERS (I think).

I also removed the hardcoded tailwind classes from select.html, and added these classes to the default_styles. Which should work as usual, and give people the posibility to easily overwrite these classes.

@GitRon
Copy link
Contributor

GitRon commented Feb 13, 2024

@blasferna Sounds good! 👌

@Thutmose3 Sounds good as well! 🚀

@Thutmose3
Copy link
Contributor Author

This PR has 2 features basically:

  • Possibilty to overwrite default_styles from settings.py
  • Move away from hardcoded Tailwind classes in templates (only select.html has been done at the moment)

@carltongibson
Copy link
Contributor

carltongibson commented Feb 13, 2024

@smithdc1 @carltongibson What do you think about this approach? I'm still the new kid on the block 🙃

@GitRon Sorry for the slow reply. I don't have a concrete preference. Goal is to parameterise the classes used. So OK. Having one clear, documented, way to do that is the answer. Beyond that, all I can say, What's the API you want to use? Keep it clean. Head there. (That's very high-level I know.)

"docs or it didnt happen"

💯

Update: I realise it was only two hours, so I won't worry about the slow reply 😅

@Thutmose3
Copy link
Contributor Author

@GitRon i added documentation to explain this new feature, there is still some work to improve on this, but if it's merged, i'll continue to expand on this feature.

It seems the tests are failing, do you know why or can point me in the good direction?

@smithdc1
Copy link
Member

Hi all 👋

Having one clear, documented, way to do that is the answer.

Completely agree. Styles can be customised already, but it's not documented. So that doesn't count. That's on me.

The code currently allows you to customise by setting the styles on the form helper. Here's the draft docs for that.

https://github.com/smithdc1/crispy-tailwind/blob/c5941952ceb1e05e84634a12b1244fbd14b504d2/docs/custom.txt#L71

Happy to go with whichever option you think is best.

Another thought, Textual/rich does css in python code? Any inspiration from there? (Likely not but...)

@Thutmose3
Copy link
Contributor Author

@smithdc1 i saw the CSSContainer class, but i was unable to get it to work as i could not find good documentation about it. And it seems it will only work when using FormHelper, which only work when doing {% crispy form %} and not with {{ form|crispy }} if i'm not mistaken?

@smithdc1
Copy link
Member

Thats a good point and seems about right to me. 👍

FormHelper gives more flexibility. It allows customising of styles per form. I imagine being able to switch between light and dark modes for example depending on a user setting. (Does that sound right? 🤔)

I wonder if there are any other considerations to take into account. I'll have a ponder tomorrow.

@Thutmose3
Copy link
Contributor Author

Thutmose3 commented Feb 13, 2024

Yeah, i use FormHelper only on big/complex forms, but when starting on a new form i dont do it, only when it becomes necessary.

The CSSContainer is handy as you can customize each form with different styles. But it's a bit more complex to set up.

By configuring it in CRISPY_TAILWIND_STYLE it is applied on all forms uniformly, but is a bit less flexible and simpler.
But in my opinion forms should all have the same styling across a project anyway, it's like "configure and forget".

Both have pros/cons, but I think both can be used. Possibility to have a default config, and be able to update it on complex forms with CSSContainer.

@carltongibson
Copy link
Contributor

@Thutmose3 That sounds plausible. If you could put that thought into a few points for the docs I think it would make a good clarification. 👍

@GitRon
Copy link
Contributor

GitRon commented Feb 16, 2024

@Thutmose3 I agree with @carltongibson.

If you could put that thought into a few points for the docs I think it would make a good clarification. 👍

Please let us know when you've done that, then we can have a review-look and continue from there. Would that be fine for you?

@Thutmose3
Copy link
Contributor Author

@carltongibson @GitRon yes will do for sure, my newborn son arrived yesterday so i'm a bit busy at the moment, but will do it once i have some more free time!

@Thutmose3
Copy link
Contributor Author

Thutmose3 commented Feb 24, 2024

Hi all, i just finished updating the documentation for this, can you let me know if this is ok for you?

I'm using Flowbite theme for Tailwind, and i think with this setup, we could have different "default_styles" dicts in the backend with different Tailwind themes, and people can configure a setting for what theme they want and it will take these classes.

But thats for another PR, once this is merged i will continue working on making this work on more fields, and cleanly move away from the "hardcoded" nature of the project.

@Thutmose3
Copy link
Contributor Author

@GitRon, what do you think, is it ok to merge?

@GitRon
Copy link
Contributor

GitRon commented Feb 27, 2024

Hi @Thutmose3! Hope your son is well? 🙂

I'm very busy at the moment but your are not forgotten. I'll have a look at it as soon as I find a couple of quite minutes.

@Thutmose3
Copy link
Contributor Author

Yes doing wonderfull thanks 🙂 ok thanks!

Copy link
Contributor

@GitRon GitRon left a comment

Choose a reason for hiding this comment

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

Hi @Thutmose3! Sorry for the late reply! Great work, only a few things popped up!

{% load l10n %}

<div class="relative">
<select class="bg-white focus:outline-none border {% if field.errors %}border-red-500 {% else %}border-gray-300 {% endif %}rounded-lg py-2 px-4 block w-full appearance-none leading-normal text-gray-700" name="{{ field.html_name }}" {{ field|build_attrs }}>
<select class="{{ field|tailwind_field_class }}{% if field.errors %} border-red-500{% endif %}" name="{{ field.html_name }}" {{ field|build_attrs }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: that sounds like a solid idea! 👌

Copy link
Contributor

Choose a reason for hiding this comment

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

question: why did you only change the select field, shouldn't this apply to all elements? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, it's just a starting point?

"error_border": "border-red-500",
}

tailwind_styles = {**default_styles, **getattr(settings, "CRISPY_TAILWIND_STYLE", {})}
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: personally, I'm a big fan of inline-comments. you could add some to explain whats going on here.

# Merging default styles with declaration from settings

for example.

The same ges for all blocks of code in this file.

The second way is to configure ``CSSContainer`` on a specific form. This will only work for forms that use FormHelper.
This allows you to override a specific form, this is usefull for unique/complex forms.

The idea is that you can easily configure all forms by overriding CRISPY_TAILWIND_STYLE in settings, and if you want to customize a specific form, you can use CSSContainer for that specific form.
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: well-written 👍

-----------------------------------------------------------------------------

Example:
``CRISPY_TAILWIND_STYLE = {
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: github docs complains here about something

This is currently only working for the input fields and the select field. More coming soon.

These are the fields you can override:
CRISPY_TAILWIND_STYLE = {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: might be worth marking it as python code?

"selectdate": "",
"error_border": "",
}

Copy link
Contributor

Choose a reason for hiding this comment

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

issue: github actions docs complains

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.

None yet

5 participants