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 select.html - URGENT #153

Merged
merged 9 commits into from
Feb 13, 2024
Merged

Conversation

Thutmose3
Copy link
Contributor

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

@Thutmose3 Thutmose3 changed the title Update select.html Update select.html - URGENT Feb 4, 2024
@GitRon
Copy link
Contributor

GitRon commented Feb 6, 2024

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
Copy link

dpjanes commented Feb 7, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

GitRon commented Feb 8, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

GitRon commented Feb 10, 2024

@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
Copy link
Contributor

GitRon commented Feb 10, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

GitRon commented Feb 11, 2024

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
Copy link
Contributor Author

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

@GitRon
Copy link
Contributor

GitRon commented Feb 11, 2024

@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
Copy link
Contributor Author

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
Copy link
Contributor

GitRon commented Feb 12, 2024

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 GitRon mentioned this pull request Feb 13, 2024
@GitRon GitRon closed this in #160 Feb 13, 2024
@GitRon GitRon reopened this Feb 13, 2024
@GitRon GitRon merged commit 6c81c79 into django-crispy-forms:main Feb 13, 2024
16 checks passed
@GitRon GitRon mentioned this pull request Feb 13, 2024
@GitRon
Copy link
Contributor

GitRon commented Feb 13, 2024

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

Hope it works and thx for the effort! 💪

@Thutmose3
Copy link
Contributor Author

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

@dpjanes
Copy link

dpjanes commented Feb 20, 2024

Late to show, but confirmed working here (1.0.3)

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.

3 participants