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

adds field for guide navigation layout type #182

Open
wants to merge 4 commits into
base: 2.x
Choose a base branch
from

Conversation

markconroy
Copy link
Member

Closes #123

What does this change?

  • Adds a new field called "List Layout" so editors can choose on a per-guide basis whether they want horizonal/vertical layout.
  • Defaults to "Vertical (Single Column)"

Screenshot 2024-11-28 at 14 53 02

How to test

Install Guides module
Create a guide overview and some guide pages
In Guide Overview, choose "Horizontal" or "Vertical" for the "List Layout"
Check that the Guide Nav responds accordingly NOTE it will need this PR from LocalGov base which has the required CSS.


Thanks to Big Blue Door for sponsoring my time to work on this.

Copy link
Member

@msayoung msayoung left a comment

Choose a reason for hiding this comment

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

Causes errors on pre-existing guides.

InvalidArgumentException: Field localgov_guides_list_layout is unknown.

@stephen-cox
Copy link
Member

Causes errors on pre-existing guides.

InvalidArgumentException: Field localgov_guides_list_layout is unknown.

You're going to need to either check the field exists before using it or add an update hook to install this into existing sites.

@markconroy
Copy link
Member Author

@msayoung @stephen-cox this PR is updated now to check for the field before acting on it.

Do we want to add this field to existing installs as well as new installs?

@markconroy markconroy requested a review from msayoung December 13, 2024 16:17
@stephen-cox
Copy link
Member

I will leave the decision on whether to add this to existing installs to someone else, but I not it's not straightforward to add is manually so might be good to push it out if it's useful.

Looks fine to me now, but before we can merge this we'll want all the tests passing.

@msayoung msayoung self-requested a review December 17, 2024 11:52
@msayoung
Copy link
Member

Also just tested and working fine. Good question about existing sites, I agree that probably yes ...?

@stephen-cox
Copy link
Member

Discussing at Merge Tuesday - would a site wide setting for this be better?

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.

Consider vertical navigation for guides
3 participants