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

[#1316] Add title-description to banner plugin #610

Merged

Conversation

vaszig
Copy link
Contributor

@vaszig vaszig commented May 9, 2023

Discussed with Alex as well, this does not make sense to be combined in one plugin. We need the title and description to be available in another section (in the main-grid and not in the banner), so a second plugin was created for this purpose.

@vaszig vaszig marked this pull request as draft May 9, 2023 14:16
@vaszig vaszig force-pushed the feature/1316-add-title-description-to-banner-plugin branch from 0c60c62 to d26e669 Compare May 10, 2023 12:07
@codecov-commenter
Copy link

codecov-commenter commented May 10, 2023

Codecov Report

❗ No coverage uploaded for pull request base (develop@f6fe153). Click here to learn what that means.
The diff coverage is 97.14%.

@@            Coverage Diff             @@
##             develop     #610   +/-   ##
==========================================
  Coverage           ?   96.41%           
==========================================
  Files              ?      579           
  Lines              ?    19951           
  Branches           ?        0           
==========================================
  Hits               ?    19235           
  Misses             ?      716           
  Partials           ?        0           
Impacted Files Coverage Δ
src/open_inwoner/conf/base.py 95.37% <ø> (ø)
src/open_inwoner/cms/banner/models.py 86.66% <85.71%> (ø)
src/open_inwoner/cms/banner/cms_plugins.py 100.00% <100.00%> (ø)
src/open_inwoner/cms/banner/forms.py 68.75% <100.00%> (ø)
...r/cms/banner/migrations/0003_auto_20230511_1125.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@vaszig vaszig marked this pull request as ready for review May 10, 2023 12:52
Comment on lines 9 to 13
name = models.CharField(
_("Name"),
max_length=250,
help_text=_("The name of the image (this will not be shown on the page)"),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need a name-field if we're not showing it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that we do not have any required fields, so maybe I should remove name and make image a required field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed with Bart, name fields are not useful. Image and title will be the required fields for the two models (BannerImage and BannerText).

Copy link
Member

Choose a reason for hiding this comment

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

Reasoning why I didn't request to remove 'name' is that I often see plugins with an (internal) name which is only used for plugin.str , this to make the name visible in the CMS when editting the page.

This is mostly used for CMS pages with long lists of plugins, so I agree that the benefit for only the bannerplugin is quite limited.

Copy link
Contributor

@jiromaykin jiromaykin left a comment

Choose a reason for hiding this comment

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

So this text area is really only visible when logged in? Becasue when you're logged out, there is a different 'welkom' area visible that is still there and cannot be removed by the user and is configarable from the Admin.

So when am logged out i still see the 'Koptekst homepage' area on Home because that is a required field - i am not sure if that one needs to be required and visible still - what do you think @alextreme ?
Screenshot 2023-05-11 at 10 21 59

@vaszig vaszig force-pushed the feature/1316-add-title-description-to-banner-plugin branch from d26e669 to e474442 Compare May 11, 2023 09:36
@alextreme
Copy link
Member

So this text area is really only visible when logged in? Becasue when you're logged out, there is a different 'welkom' area visible that is still there and cannot be removed by the user and is configarable from the Admin.

So when am logged out i still see the 'Koptekst homepage' area on Home because that is a required field - i am not sure if that one needs to be required and visible still - what do you think @alextreme ?

This was discussed with @vaszig as a workable intermediate solution (populating the anonymous page via the Algemene Configuratie, and the logged-in homepage via the CMS). But I agree it isn't ideal, I would like to have the anonymous homepage configurable via the CMS too but that would be a next step.

@vaszig vaszig force-pushed the feature/1316-add-title-description-to-banner-plugin branch from e474442 to 48b174a Compare May 11, 2023 14:14
@Bartvaderkin Bartvaderkin self-requested a review May 15, 2023 09:03
Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

Last week I started adding tests for the plugins, for example (some others as well):

https://github.com/maykinmedia/open-inwoner/blob/develop/src/open_inwoner/cms/products/tests/test_plugin_product_location.py

We should do that for all plugins.

@alextreme alextreme merged commit 03dc7e0 into develop May 15, 2023
@alextreme alextreme deleted the feature/1316-add-title-description-to-banner-plugin branch May 15, 2023 12:11
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.

5 participants