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

Quick: Avoid Coupling Bootstrap and Generic Link #138

Conversation

wesleyboar
Copy link
Contributor

@wesleyboar wesleyboar commented Aug 31, 2021

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.
  2. A user can do so, by following instructions like these:
  3. When a user wants to use Generic "Image" plugin and Bootstrap "Picture / Image" plugin, the solution is simpler:
  4. The solution for Generic "Link" plugin and Bootstrap "Link / Button" plugin, the solution is not as simple:

@wesleyboar wesleyboar marked this pull request as ready for review August 31, 2021 15:53
@NicolaiRidani NicolaiRidani requested a review from a team October 29, 2021 14:19
@marksweb marksweb self-requested a review November 2, 2021 16:28
Copy link
Member

@marksweb marksweb left a comment

Choose a reason for hiding this comment

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

I'm a fan of consistency, so thanks for the contribution 👍

@crydotsnake
Copy link
Contributor

Can i merge it ? @marksweb

@marksweb
Copy link
Member

marksweb commented Nov 2, 2021

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 crydotsnake merged commit 713c1c8 into django-cms:master Nov 2, 2021
@crydotsnake
Copy link
Contributor

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

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

marksweb commented Nov 2, 2021

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

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.

None yet

3 participants