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

Added subsites_menu region. #542

Merged
merged 9 commits into from May 21, 2024
Merged

Added subsites_menu region. #542

merged 9 commits into from May 21, 2024

Conversation

rupertj
Copy link
Member

@rupertj rupertj commented May 2, 2024

No description provided.

@justinepocock justinepocock marked this pull request as ready for review May 13, 2024 11:00
@dedavidson dedavidson self-requested a review May 16, 2024 16:21
Copy link

@dedavidson dedavidson left a comment

Choose a reason for hiding this comment

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

I feel the following commits would be better in the subsite extras module as documentation/examples rather than being merged into the base theme:

@justinepocock
Copy link
Member

I was in two minds about that myself.

@willguv willguv requested a review from markconroy May 17, 2024 12:35
@markconroy
Copy link
Member

Hi,

I'd prefer the HTML and CSS for this to be in this theme rather than in the module. We can add HTML and CSS to the module as well if we want so that it works even if localgov_base is not the base theme being used.

Keep the variables here means we can place them all in the variables.css file for consistency, and it will make it easier for sub-themes to find them and override them.

@dedavidson
Copy link

Hi @markconroy
The HTML and CSS in the three commits are examples though and don't do anything unless a menu called 'subsites' is created. They will have to be overridden/re-created for whatever the specific implementation calls its menus.

@markconroy
Copy link
Member

Hi @dedavidson,

Yep, I've noticed that. I'm doing a rewrite of this PR at the moment, hoping to have something ready for it by EOD today, if not, I'll definitely have it ready over the weekend.

@rupertj
Copy link
Member Author

rupertj commented May 17, 2024

The HTML and CSS in the three commits are examples though and don't do anything unless a menu called 'subsites' is created. They will have to be overridden/re-created for whatever the specific implementation calls its menus.

Subsites extras does create a menu called 'subsites' by default on install now.

@markconroy
Copy link
Member

markconroy commented May 17, 2024

I've a pretty major re-write of this done now if anyone wants to review it.

I've no JS done yet. Let's get this reviewed and merged and I'll get the JS done then.

Also, related PR: localgovdrupal/localgov_scarfolk#36 (it's just one line of code)

@markconroy
Copy link
Member

A few small updates since Friday, and the JS added for the menu to toggle on/off on small screens.

@dedavidson dedavidson dismissed their stale review May 20, 2024 08:31

out of date

Copy link
Member

@justinepocock justinepocock left a comment

Choose a reason for hiding this comment

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

cheers mark

@markconroy markconroy merged commit 94c9baf into 1.x May 21, 2024
8 checks passed
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

4 participants