search

django-cms / djangocms-bootstrap4

django CMS Bootstrap 4 is a plugin bundle for django CMS providing several components from the popular Bootstrap 4 framework.
https://www.django-cms.org/
Other
82 stars 58 forks source link

Quick: Avoid Coupling Bootstrap and Generic Link #138

Closed wesleyboar closed 2 years ago

wesleyboar commented 2 years ago

Goal

Do not couple Bootstrap Link plugin to Generic link plugin.

Changes

  1. In Link plugin, mimic how Picture plugin creates fieldsets from corresponding generic plugin.

Testing

Manual testing:

  1. Have a CMS that installs Bootstrap4 Link and Picture plugin.
  2. Edit code to support Bootstrap and Generic Link and Picture plugin.
  3. Remove extra code for supporting Link plugins:
  4. Add and save these plugins:
    • "Generic" > "Link"
    • "Bootstrap4" > "Link"

Background

Supporting (A) Generic "Link" plugin and Bootstrap "Link / Button" plugin takes more code than (B) Generic "Image" plugin and Bootstrap "Picture / Image" plugin, because Bootstrap Link plugin points to Generic Link plugin fieldsets, instead of cloning them (like Bootstrap Picture plugin).

1. If a user wants to use Generic "Link" plugin __and__ Bootstrap "Link / Button" plugin, they cannot immediately do so. - https://github.com/django-cms/djangocms-link/issues/163 - https://github.com/django-cms/djangocms-link/issues/167 2. A user can do so, by following instructions like these: - https://github.com/django-cms/djangocms-link/issues/163#issuecomment-868818891 - https://github.com/TACC/Core-CMS/pull/278 3. When a user wants to use Generic "Image" plugin __and__ Bootstrap "Picture / Image" plugin, the solution is simpler: - https://github.com/TACC/Core-CMS/blob/884194e/taccsite_cms/contrib/bootstrap4_djangocms_picture/cms_plugins.py#L5-L11 4. The solution for Generic "Link" plugin __and__ Bootstrap "Link / Button" plugin, the solution is not as simple: - https://github.com/TACC/Core-CMS/blob/884194e/taccsite_cms/contrib/bootstrap4_djangocms_link/cms_plugins.py#L14-L28
crydotsnake commented 2 years ago

Can i merge it ? @marksweb

marksweb commented 2 years ago

Can i merge it ? @marksweb

@crydotsnake Sure.

Screenshot_20211102_164307.jpg

Do you see this squash and merge option? This is what I was talking about on your docs PR. This option creates 1 commit for the PR in the target branch.

crydotsnake commented 2 years ago

Can i merge it ? @marksweb

@crydotsnake Sure.

Screenshot_20211102_164307.jpg

Do you see this squash and merge option? This is what I was talking about on your docs PR. This option creates 1 commit for the PR in the target branch.

Yes, I was blind at the first moment and saw it only afterwards 🙈.

crydotsnake commented 2 years ago

@marksweb Oops... I think some tests failed. But that was shown to me only after the merge.

https://github.com/django-cms/djangocms-bootstrap4/runs/4082975004?check_suite_focus=true

marksweb commented 2 years ago

@marksweb Oops... I think some tests failed. But that was shown to me only after the merge.

https://github.com/django-cms/djangocms-bootstrap4/runs/4082975004?check_suite_focus=true

Ah I'm on my phone so can't see checks & actions in the app.