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

feat: Adds BS5 modal components #234

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Conversation

mavoIn
Copy link

@mavoIn mavoIn commented Aug 30, 2024

As discussed in #233 I added a modal component. It works like the cards component. Basic documentation added as well.

Copy link

codecov bot commented Aug 30, 2024

Codecov Report

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.

Files with missing lines Patch % Lines
djangocms_frontend/contrib/modal/forms.py 0.00% 47 Missing ⚠️
djangocms_frontend/contrib/modal/cms_plugins.py 0.00% 45 Missing ⚠️
djangocms_frontend/contrib/modal/models.py 0.00% 26 Missing ⚠️
...ms_frontend/contrib/modal/frameworks/bootstrap5.py 0.00% 13 Missing ⚠️
djangocms_frontend/contrib/modal/constants.py 0.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #234      +/-   ##
==========================================
- Coverage   86.93%   82.96%   -3.97%     
==========================================
  Files         120      125       +5     
  Lines        3107     3252     +145     
  Branches      330      337       +7     
==========================================
- Hits         2701     2698       -3     
- Misses        304      452     +148     
  Partials      102      102              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fsbraun
Copy link
Member

fsbraun commented Aug 30, 2024

So coool! 😎 I will look into it! Can you get the pre-commit stuff sorted out meanwhile?

@mavoIn
Copy link
Author

mavoIn commented Aug 30, 2024

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

@fsbraun
Copy link
Member

fsbraun commented Aug 30, 2024

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
  • Add tests in the 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.

@mavoIn
Copy link
Author

mavoIn commented Aug 30, 2024

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

Copy link
Member

@fsbraun fsbraun left a comment

Choose a reason for hiding this comment

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

I think I need to better understand how to use the modal? Maybe you can give an example in the docs?

docs/source/components.rst Show resolved Hide resolved
@fsbraun
Copy link
Member

fsbraun commented Sep 11, 2024

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.

@mavoIn
Copy link
Author

mavoIn commented Sep 12, 2024

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

tests/modal/test_models.py Outdated Show resolved Hide resolved
@mavoIn mavoIn requested a review from fsbraun November 9, 2024 22:25
@fsbraun
Copy link
Member

fsbraun commented Nov 10, 2024

@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.

Copy link
Member

@fsbraun fsbraun left a comment

Choose a reason for hiding this comment

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

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?

mavoIn and others added 2 commits November 12, 2024 22:11
Change parent_classes CardPlugin to ModalPlugin
@mavoIn
Copy link
Author

mavoIn commented Nov 12, 2024

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

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