Open mavoIn opened 2 months ago
Attention: Patch coverage is 0%
with 136 lines
in your changes missing coverage. Please review.
Project coverage is 82.96%. Comparing base (
f11feee
) to head (bc44590
). Report is 35 commits behind head on master.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
So coool! 😎 I will look into it! Can you get the pre-commit stuff sorted out meanwhile?
Hi @fsbraun, it is my first pull request ever. Maybe you have to look over the code twice. 😇 I tested it locally. Meanwhile I use the forked version. Best mavoIn
Hey @mavoIn ! Can I ask you to do the following things:
pip install ruff
(if you haven't done so already) and ruff check --fix djangocms_frontend
tests
folder. Please feel free to take similar tests as a blueprint. The tests are not very sophisticated but just check if the model and the rendering is working.I hopefully will be able to test the modal in a pet project tomorrow.
HI @fsbraun , Unfortunately, I have only been able to prepare a few test files for you. They are not complete. I am currently busy learning more. :D
Also, can you take a look at the import issue showing up in tests? And the migrations seem not up-to-date. You may update the initial migration - they are DB no-ops anyways.
Hello @fsbraun, thanks a lot for all your advices. I added the missing migrations and updated the documentation.
There are problems importing the tests because I was only able to insert a code framework from the collapse example. Please let me know so that you can fix the tests or I might have to ask friends for further support.
Best mavoIn
@mavoIn I see you have updated quite a few things! Thank you. Still, the tests are failing. Try running them locally: python ./run_tests.py
. There seems to be an import issue, an attribute issue and maybe some tweaking for the migrations required.
I have some concerns about the structure:
- Modal triggers should be possible anywhere on a page
- The Modal plugin is not needed as far as I can see. I think the
<div class="modal"></div>
can go into the modal container.- I still would need to understand a use case with a static trigger. Can you give an exmaple?
Hi @fsbraun, what do you exactly mean with static trigger?
Do you mean the requirement of the trigger inside the modal component?
From my perspective i want to prevent user errors by using the loosely coupled modal trigger like it is done in the collapse example.
Best mavo
As discussed in #233 I added a modal component. It works like the cards component. Basic documentation added as well.