django-crispy-forms / crispy-tailwind

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

Update select.html - URGENT #153

Closed Thutmose3 closed 4 months ago

Thutmose3 commented 5 months ago

This line was removed in a previous commit, which breaks the HTML of the whole page

GitRon commented 4 months ago

Hi @Thutmose3!

Thx for the PR! The div seems to be astray - there is only one div tag being opened and we close two.

Can you confirm that the error isn't in some other part of your application?

Best regards
Ronny

dpjanes commented 4 months ago

I can confirm the issue. Strongly suggest that instead of having isolated </div> in e.g. select.html they all be moved up to field.html.

Thutmose3 commented 4 months ago

Yes, there is something strange with these divs, it might slip under the radar for some people as the browser fixes this issue sometimes on the fly, but sometimes it can't and then you have broken html/website

GitRon commented 4 months ago

Thx for your input! Anybody up for a second round of PR? I think adding a stray div in the select.html isn't clean, right?

Thutmose3 commented 4 months ago

Ok i fixed all astray div's in field.html, and corresponding included files. I think in the future D or https://github.com/Riverside-Healthcare/djLint pre-commit hooks should be used to prevent these mistakes.

GitRon commented 4 months ago

@Thutmose3 Thx for your effort, I'll have a look the next days! And totally agree on the HTML linter - though it wouldn't have fixed this specific issue, right?

Since djlint claims to be unmaintained and I've had a very fruitful improvement disussion with the maintainer of DjHTML a couple of months ago, I'd go for this one. Agreed?

GitRon commented 4 months ago

Supplemental: The code looks good and this would explain the original issue. But some tests fail now. Could you have a look why?

Best
Ronny

Thutmose3 commented 4 months ago

I like bot DjHTML and djlint, so good for me.

For the tests i tried to track down the issue, but it's super strange. It seems that when the divs are wrong the tests pass, but when the divs are correct, the tests fail. So i think the tests are wrong or something, you maybe have better insigts on how these tests work?

GitRon commented 4 months ago

For the tests i tried to track down the issue, but it's super strange. It seems that when the divs are wrong the tests pass, but when the divs are correct, the tests fail. So i think the tests are wrong or something, you maybe have better insigts on how these tests work?

I checked your branch and I had to merge the latest main to make it reproducible. Very odd!

Nevertheless, if you look at the HTML output in the tests, we do have a stray closing div tag there, so the tests fail for the right reason. But I don't get, where this is coming from...

Update: @Thutmose3 Please merge the current main branch and look at the select.html, there I found a stray closing div. When I remove this, the tests pass locally.

Thutmose3 commented 4 months ago

Ok i just merged main into my branch, and removed that stray div. Tests are not starting automatically, how come?

GitRon commented 4 months ago

@Thutmose3 awesome! And in this repo, somebody has to approve pipelines. Guess, to avoid running into throttling from github.

Just started it, though. Let's see what happens!

Thutmose3 commented 4 months ago

Ok, it all seems to pass now!

Other question, why do we do "{% include 'tailwind/layout/select.html' %}" when it's only called once in the entire project? Can we just straight add it to field.html to adhere to LOB?

GitRon commented 4 months ago

Hi @Thutmose3

I think the idea is that everybody can overwrite this element as simple as possible so they put it in a separate file.

I'll double check the changes in here soon and prepare a release soon. 👌

GitRon commented 4 months ago

Ok @Thutmose3! v1.0.3 is on PyPI: https://pypi.org/project/crispy-tailwind/#history

Hope it works and thx for the effort! 💪

Thutmose3 commented 4 months ago

I upgraded, and it seems to work properly, will report back if i see more issues

dpjanes commented 4 months ago

Late to show, but confirmed working here (1.0.3)