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

Feature/110 Add content local task for alert banners #111

Merged
merged 3 commits into from
Apr 14, 2021
Merged

Feature/110 Add content local task for alert banners #111

merged 3 commits into from
Apr 14, 2021

Conversation

tebb
Copy link

@tebb tebb commented Feb 2, 2021

I have added the 'Alert banners' local task and tested that it appears after routing and links cache is flushed.

I added a second commit which (I believe) corrects some capitalisation and a typo. I wasn't sure if it's OK to do that in this specific issue.

@andybroomfield
Copy link
Contributor

@tebb is this reviewable?

@tebb
Copy link
Author

tebb commented Mar 18, 2021

Thanks @andybroomfield - Yes please.

(We thought that creating pull requests automatically queued things for review. Should we be requesting code reviews?)

@andybroomfield andybroomfield self-requested a review March 22, 2021 22:55
Copy link
Contributor

@andybroomfield andybroomfield left a comment

Choose a reason for hiding this comment

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

I've installed this and it worked fine at first, however the tabs are missing when a user goes to the manage alert banners page.
Will post screenshots shortly.

Looking at how the /admin/content/media and /admin/content/files views work, I think the tab has to be in the view, and the menu item should be added to localgov_alert_banner.links.menu.yml.

Copy link
Contributor

@andybroomfield andybroomfield left a comment

Choose a reason for hiding this comment

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

Thanks for your work @tebb and apologies for late reviewing.
I had a look and overall good, however there are some quirks I spotted with the tabs not showing once I go to the manage alert banner page. See inline comments.

title: Delete
weight: 10

entity.localgov_alert_banner.collection:
route_name: view.localgov_admin_manage_alert_banners.localgov_alert_banner_admin_list
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change the route name to entity.localgov_alert_banner.collection
Theres a weird bug that when you go to the alert banner page the tabs no longer show. I think its to do with defining a collection route in the entity class, but we don't use the collection, we use the view. This causes issues with the menu / tab display but we will sort these out in another issue.


entity.localgov_alert_banner.collection:
route_name: view.localgov_admin_manage_alert_banners.localgov_alert_banner_admin_list
base_route: system.admin_content
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the extra space
base_route: system.admin_content

entity.localgov_alert_banner.collection:
route_name: view.localgov_admin_manage_alert_banners.localgov_alert_banner_admin_list
base_route: system.admin_content
title: 'Alert banners'
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to add weight: 50 to the bottom so it shows last in list.

…rs page

Need to use the entity.localgov_alert_banner.collection route not the view.
@andybroomfield
Copy link
Contributor

I've added the fixes, if theres anyone who can give this a review before I merge.
Then I can get a release prepped.

Copy link
Collaborator

@danchamp danchamp left a comment

Choose a reason for hiding this comment

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

Looks good, tested and working as expected.

@andybroomfield
Copy link
Contributor

Thanks @danchamp

@andybroomfield andybroomfield merged commit b3a1976 into localgovdrupal:master Apr 14, 2021
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.

3 participants