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.

Sign up for GitHub

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

Jump to bottom

Create an 'edit' popular links page #2218

Merged
merged 4 commits into from
Jun 27, 2024
Merged

Create an 'edit' popular links page #2218

merged 4 commits into from
Jun 27, 2024

Conversation

anatron
Copy link
Contributor

@anatron anatron commented Jun 24, 2024

Trello card

This PR will allow us to edit existing popular links in publisher.
This includes:

  • Adding the sidebar on the show page which contains the 'Create new edition' button.
  • Adding the create new edition page, which includes the 'Edit popular links' button and 'Publish' button in the sidebar.
  • Adding the edit page, which includes the options to edit and save.
  • Adding the controllers and routes for create, edit, update and publish.
  • Adding validation to the models for invalid URLs
  • Integration, functional and model tests for create, edit, update and publish.
  • Adding publish_popular_links to the workflow and action model.

@syed-ali-tw syed-ali-tw force-pushed the edit-pop branch 2 times, most recently from d2b6d2c to 4efb28e Compare June 24, 2024 16:31
mtaylorgds
mtaylorgds requested changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@mtaylorgds mtaylorgds left a comment

Choose a reason for hiding this comment

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

Good work!

Could use a few minor changes before merging, the biggest ones are around using the components from the publishing components gem wherever possible.

app/views/homepage/popular_links/_sidebar.html.erb Outdated Show resolved Hide resolved
app/models/popular_links_edition.rb Outdated Show resolved Hide resolved
app/controllers/homepage_controller.rb Outdated Show resolved Hide resolved
app/controllers/homepage_controller.rb Show resolved Hide resolved
app/helpers/popular_links_helper.rb Outdated Show resolved Hide resolved
test/integration/homepage_popular_links_test.rb Outdated Show resolved Hide resolved
test/integration/homepage_popular_links_test.rb Outdated Show resolved Hide resolved
test/integration/homepage_popular_links_test.rb Outdated Show resolved Hide resolved
test/integration/homepage_popular_links_test.rb Outdated Show resolved Hide resolved
test/models/popular_links_edition_test.rb Show resolved Hide resolved
@davidtrussler
Copy link
Contributor

@anatron @syed-ali-tw Is there a design available for the new sidebar? I wonder as well if this is intended to be sticky rather than staying fixed at the top when the user scrolls down.

@anatron anatron force-pushed the edit-pop branch 3 times, most recently from 89e1c9f to 371c3b9 Compare June 26, 2024 14:33
@anatron anatron force-pushed the edit-pop branch 2 times, most recently from 174d21b to f2485a9 Compare June 26, 2024 16:08
@anatron
Copy link
Contributor Author

anatron commented Jun 26, 2024

@anatron @syed-ali-tw Is there a design available for the new sidebar? I wonder as well if this is intended to be sticky rather than staying fixed at the top when the user scrolls down.

Yes, it is intended to be sticky. This will be part of another PR.

mtaylorgds
mtaylorgds approved these changes Jun 27, 2024
View reviewed changes
Copy link
Contributor

@mtaylorgds mtaylorgds 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 from my perspective. Great job!

@davidtrussler
Copy link
Contributor

@anatron @syed-ali-tw Is there a design available for the new sidebar? I wonder as well if this is intended to be sticky rather than staying fixed at the top when the user scrolls down.

Yes, it is intended to be sticky. This will be part of another PR.

OK, though it should be a fairly simple CSS addition if you think it's worth doing it as part of this work.

davidtrussler
davidtrussler approved these changes Jun 27, 2024
View reviewed changes
Copy link
Contributor

@davidtrussler davidtrussler left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@syed-ali-tw syed-ali-tw merged commit 8811da5 into main Jun 27, 2024
12 checks passed
@syed-ali-tw syed-ali-tw deleted the edit-pop branch June 27, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@syed-ali-tw syed-ali-tw syed-ali-tw left review comments

@mtaylorgds mtaylorgds mtaylorgds approved these changes

@davidtrussler davidtrussler davidtrussler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@anatron @davidtrussler @syed-ali-tw @mtaylorgds