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

497 edit page related external links #2490

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

davidtrussler
Copy link
Contributor

@davidtrussler davidtrussler commented Jan 9, 2025

Trello

This gives the user the ability to add/delete/edit external links to a document. It uses the Add another component, which provides a different UI in JavaScript and non-JavaScript scenarios (shown below).

One aspect of the Acceptance Criteria that is not covered so far is to display an error message when both the "Title" and "URL" fields are blank: it continues to follow the current behaviour of displaying a success message while not saving the changes. This proved problematic because in a non-JS scenario it is in fact acceptable to submit empty fields because these are always provided by the component and may not be used.

View JS disabled JS enabled
On page load: document has no existing external links Screenshot 2025-01-09 at 17 10 01 Screenshot 2025-01-09 at 17 11 24
"Add another link" clicked Screenshot 2025-01-09 at 17 17 59
"Delete" clicked Screenshot 2025-01-09 at 17 23 57
"Delete" clicked Screenshot 2025-01-09 at 17 25 04
"Saved" clicked publisher dev gov uk_editions_672dffe2c5d19b00398affd7_related_external_links publisher dev gov uk_editions_672dffe2c5d19b00398affd7_related_external_links_2

@davidtrussler davidtrussler force-pushed the 497_Edit-page_Related-external-links branch 5 times, most recently from 30a73c3 to 61b158b Compare January 9, 2025 15:24
@davidtrussler davidtrussler marked this pull request as ready for review January 10, 2025 09:47
Copy link
Contributor

@ellohez ellohez left a comment

Choose a reason for hiding this comment

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

LGTM! Just one tiny comment

@@ -23,6 +23,8 @@ def current_tab_name
"unpublish"
when "admin"
"admin"
when "related_external_links"
"linking"
Copy link
Contributor

Choose a reason for hiding this comment

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

Might just be me but the name 'linking' seems a little different to the other tabs. Would 'external_links' be more in keeping with the other tab names and match up more with the name of the property in the Artefact model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might just be me but the name 'linking' seems a little different to the other tabs. Would 'external_links' be more in keeping with the other tab names and match up more with the name of the property in the Artefact model?

Yeah that's been bugging me a bit since the start. I took the name from the routes file that appears to redirect related_external_links in the URL to linking in the views but I have no idea why. Happy to hear further opinions on that though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can use whatever we want here, as it's not really user-visible. So long as we make sure we update all the places that might reference it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed linking to related_external_links in all appropriate places. I think it reads better like that.

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.

When the external links are invalid, the invalid values are not persisted in the text boxes, the text boxes are simply populated with whatever the currently-saved external links are. This appears to be the existing behaviour…not sure whether we are happy to keep this behaviour for now?

On my machine, the delete button has no space above it:
image

app/assets/stylesheets/application.scss Show resolved Hide resolved
test/functional/editions_controller_test.rb Outdated Show resolved Hide resolved
test/functional/editions_controller_test.rb Outdated Show resolved Hide resolved
test/functional/editions_controller_test.rb Outdated Show resolved Hide resolved
test/functional/editions_controller_test.rb Outdated Show resolved Hide resolved
app/controllers/editions_controller.rb Show resolved Hide resolved
test/integration/edition_edit_test.rb Outdated Show resolved Hide resolved
test/integration_test_helper.rb Outdated Show resolved Hide resolved
@davidtrussler davidtrussler force-pushed the 497_Edit-page_Related-external-links branch from 51e97e3 to 4baa904 Compare January 10, 2025 17:50
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.

A few minor comments.

test/integration/edition_edit_test.rb Outdated Show resolved Hide resolved
test/integration/edition_external_links_test.rb Outdated Show resolved Hide resolved
test/integration/edition_edit_test.rb Outdated Show resolved Hide resolved
@davidtrussler davidtrussler force-pushed the 497_Edit-page_Related-external-links branch from a5498f4 to de82064 Compare January 15, 2025 16:36
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.

I think the only outstanding issues are around how validation works, but they don't need to block this PR from being merged.

- Create view/partials to render the UI
- Update `editions_controller` to add a `update_related_external_links` action
- Update routes for `update_related_external_links` action
- Add references to the JS/CSS for the "Add another" component
- Update controller test for `update_related_external_links` method
- Update `edition_edit` test for `related external links` tab
- Add new `edition_external_links_test` integration test for JS scenarios
@davidtrussler davidtrussler force-pushed the 497_Edit-page_Related-external-links branch from aae0dbc to 4e8e915 Compare January 16, 2025 10:53
@davidtrussler davidtrussler merged commit b5b2155 into main Jan 16, 2025
12 checks passed
@davidtrussler davidtrussler deleted the 497_Edit-page_Related-external-links branch January 16, 2025 11:07
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