search

django-crispy-forms / crispy-tailwind

A Tailwind template pack for django-crispy-forms
MIT License
329 stars 56 forks source link

Fixing Multiple Issues in Select Element Rendering #150

Closed blasferna closed 4 months ago

blasferna commented 5 months ago

This pull request addresses several issues related to the rendering of select element attributes. The identified problems are enumerated below and were fixed in this pull request:

  1. Missing id Attribute The library fails to render the id attribute of the select element.

    Fixed #125

  2. Select option is inheriting attributes from the select The options are rendering the attributes from the select when they shouldn't according to the widget definition.

    Fixed #132

  3. Missing multiple and selected Attribute The select element does not render the multiple and selected attribute.

    Fixed #119

  4. Missing required Attribute

    The required attribute of the widget is not being rendered.

  5. Custom Attributes for Options

    Custom select widgets defining attributes in the create_option method or disabling some options do not render properly.

  6. Rendering of Grouped Select Options

    Grouped options are currently rendered incorrectly.

    Before

    grouped_select

    After

    grouped_select_after

  7. Django Autocomplete Light Attributes

    Custom selects that override the build_attrs method of the Widget object are not properly supported, leading to integration issues with other packages.

    Before

    The dal attributes are not rendered and because of that the select is not working properly

    dal_select_before

    After

    image

    Fixed #136

Also, this pull requests fixes #98

There is a repository where you can check the select with the fixes applied in this pull request.

blasferna commented 5 months ago

@GitRon and @smithdc1, could you please review this pull request when you have a moment and let me know if there's anything missing to move forward with it? Thank you.

GitRon commented 5 months ago

Hi @blasferna! Sorry, didnt get an email about it from GitHub! I'll have a look as soon as I find the time! Thx for your effort!

GitRon commented 5 months ago

Thx @blasferna, your changes look good!

@carltongibson @smithdc1 Whats the best way here for content review? How do you do this usually?

Best regards
Ronny

smithdc1 commented 5 months ago

Hi @GitRon

My currently preferred approach is to use the outputs from the results folder. With the tailwind playground it's fairly easy to paste the new output files there to see how it looks.

It seems that there's not currently enough test coverage to see all the changes included in this PR. I can't see any changes in the tests for multiple, selected or required, for example? 🤔

GitRon commented 5 months ago

Hi @smithdc1!

Where would I find this results folder?

And the issue about missing test cases were feedback for @blasferna, am I correct?

Furthermore, I'd suggest we document how to do Content Review (with the playground). Should help future us.

blasferna commented 5 months ago

Hi @GitRon and @smithdc1,

Just wanted to let you know that I've addressed the feedback regarding test coverage for the select attributes in the PR. I've added tests for multiple, selected, and required attributes.

Let me know if there's anything else I can do to move this forward.

Best regards, Blas

GitRon commented 5 months ago

@blasferna Amazing! I just saw the "results" directory.

The output on the tailwind playground looks good:

grafik

@smithdc1 @carltongibson @justinmayer Do you folk want to have a look as well? Otherwise, I'd merge it in a couple of days.

And @blasferna, thx for all your effort! A very comprehensive description and clean code 👌

GitRon commented 4 months ago

@smithdc1 @carltongibson @justinmayer Do you folk want to have a look as well? Otherwise, I'd merge it in a couple of days.

No news is good news. Let's get it merged 💪

anorthall commented 4 months ago

@blasferna This is incredible - thank you! <3