-
Notifications
You must be signed in to change notification settings - Fork 85
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
✨(backend) Added new page extension 'IndexPage' #2493
Open
sveetch
wants to merge
4
commits into
master
Choose a base branch
from
pageindex_extension
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jbpenrath
reviewed
Sep 18, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a first review, it looks to work great!
About the look'n feel of the menu, as there is a need of interaction I think we should manage those menus through a React component.
jbpenrath
reviewed
Sep 18, 2024
sveetch
force-pushed
the
pageindex_extension
branch
from
September 19, 2024 01:34
8e181ff
to
0566921
Compare
sveetch
force-pushed
the
pageindex_extension
branch
from
September 19, 2024 14:22
0566921
to
bd15fd9
Compare
sveetch
force-pushed
the
pageindex_extension
branch
from
September 27, 2024 00:27
bd15fd9
to
0cff3e7
Compare
jbpenrath
reviewed
Sep 30, 2024
...ie-site-factory/template/{{cookiecutter.site}}/src/backend/{{cookiecutter.site}}/settings.py
Outdated
Show resolved
Hide resolved
sveetch
force-pushed
the
pageindex_extension
branch
3 times, most recently
from
October 7, 2024 20:39
09ace4a
to
8183a26
Compare
This new extension cover the need of some options for pages in main menu. These options are: - Enable to show a dropdown of children page; - Add a custom color class for the page on hover event in menu; This implementation brings a new Page extension, new admin, new menu modifier, new extension toolbar, new settings, menu template changes and some CSS adjustments.
…seless and confusing
sveetch
force-pushed
the
pageindex_extension
branch
from
November 19, 2024 14:30
8183a26
to
0b8c545
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
This new extension cover the need of some options for pages in main menu. These options are:
Proposal
The only proper way to carry these option data on a CMS page is through a page extension.
However we can not add these options to the existing page extensions since they are not used on the pages listed on the main menu because these pages are just some index to list children pages (like categories, news, courses, etc..) and so these index pages don't have any extension yet.
So we would add this new extension 'MainMenuEntry' that will carry new options and we would create this extension on pages that are listed in the main menu, commonly these pages are on level 0 of the CMS tree.
Includes
MainMenuEntry
with its migration;MainMenuEntryExtensionToolbar
to display (or not) the toolbar menu entry to create or edit the extension on a page;MainMenuEntryAdmin
use by the related wizard;MainMenuEntryFactory
to create a page with the new extension;MenuWithMainMenuEntry
to patch menu to include the new extension data (since on default the menu node are not page objects and lack of many page attributes);Notices
Settings and defaults
We use three settings and only a single default from
defaults
module because this module is loaded too early which avoid us to change their value in tests. But as a setting we can still override these settings on the fly inside tests.INDEX_MENU_COLOR_CLASSES
manage the choice list for color class variant names that can be selected from MainMenuEntry extension. Default is empty but you can override it from settingRICHIE_INDEX_MENU_COLOR_CLASSES
;RICHIE_MAINMENUENTRY_ALLOW_CREATION
has no default variable since we need to override it from test. This settings define if granted users can add this extension on existing page or not. If true, this will be possible and if false users will only be able to add this extension when creating a new page with the related wizard form. When migrating a project to a Richie version with MainMenuEntry feature, developer would set it to true temporarily and then set it to false when finished so it follow the common Richie rule to only add extension through wizard creation;RICHIE_MAINMENUENTRY_MENU_ALLOWED_LEVEL
define allowed level to see the toolbar entry for the MainMenuEntry extension form and is also used in menu modifier to avoid working on too many nodes. This is because we commonly build main menu from pages at root level (level 0) but some users may build main menu from pages under the Home page so the pages to process would be at level 1. And finally we want to restrict using this extension everywhere in the CMS tree;Sass source for main menu
During development i've setted some dummy class variants on menu entry to be able to check and implement layout changes. And now the hover event for the menu entry use a CSS variable
--r--menu--item--hover--color
to define the background color, this should help you with your Cunningham system. The variable name is temporary, we can rename to whatever you want.Menu modifier
The menu modifier has been developed with performance in focus so it should never add more than a single more queryset to get extensions for all pages.
Menu dropdown
The current layout implementation for menu entries which allow to list children page is ugly since we don't have any real dropdown menu in Richie because we don't include any Bootstrap component and we can not correctly and quickly implement it in vanilla JavaScript since it need to have behavior for desktop mode (a simple dropdown open on hover event) and another one in mobile mode (an accordion alike opening on click event).
For this point i just don't know how we could properly manage it.
Model migration
Included migration to initialize new model MainMenuEntry has been altered so the color choices are not hardcoded in migration, this allow to change the add or remove color from settings without to create a new migration each time. This could add a drawback (if removing used color, Django won't say nothing and you can not know to append a data migration for this change) that you don't want, we can them turn back to the legacy Django behavior (keep hardcoded list on field migration).