AO3-7397 Configurable page for skin previews#5744
AO3-7397 Configurable page for skin previews#5744nicolacleary wants to merge 3 commits intootwcode:masterfrom
Conversation
nicolacleary
force-pushed
the
AO3-7397_skin_preview_page
branch
from
779a5ad to
c92baf3
Compare
| flash[:notice] << t(".remove_skin") | ||
| flash[:notice] << t(".tip") | ||
| flash[:notice] << ("<a href='#{skin_path(@skin)}' class='action' role='button'>".html_safe + t(".return_to_skin") + "</a>".html_safe) | ||
| redirect_to "#{ArchiveConfig.SKIN_PREVIEW_URL}?site_skin=#{@skin.id}" |
There was a problem hiding this comment.
This provides no guarantees that the url is correct (formatting or exists) - e.g. if you configure "/idonotexist" then you would get an error only when you tried to preview a skin.
There was a problem hiding this comment.
That's fine, but thanks for noting it!
Bilka2
left a comment
Bilka2
left a comment
There was a problem hiding this comment.
This is a full review now, but it's just one thing :D
| flash[:notice] << t(".remove_skin") | ||
| flash[:notice] << t(".tip") | ||
| flash[:notice] << ("<a href='#{skin_path(@skin)}' class='action' role='button'>".html_safe + t(".return_to_skin") + "</a>".html_safe) | ||
| redirect_to "#{ArchiveConfig.SKIN_PREVIEW_URL}?site_skin=#{@skin.id}" |
There was a problem hiding this comment.
That's fine, but thanks for noting it!
| flash[:notice] << t(".skin_title", title: @skin.title) | ||
| flash[:notice] << t(".remove_skin") | ||
| flash[:notice] << t(".tip") | ||
| flash[:notice] << ("<a href='#{skin_path(@skin)}' class='action' role='button'>".html_safe + t(".return_to_skin") + "</a>".html_safe) |
There was a problem hiding this comment.
Could you use helpers.link_to to create this link HTML so that we can avoid the html_safe call? If you need to remove the role='button' for that, that's fine, it should be removed per AO3-6765 anyway.
There was a problem hiding this comment.
Speaking of AO3-6765, the PR for it has been in "Reviewed: Action Needed" without changes for over a year, which makes it adoptable for AD&T volunteers like me. However, if I adopted it now, we'd have loads of merge conflicts between the changes here and the changes for AO3-6765.
Would you be up for adding AO3-6765 into the PR here, since you're already doing the i18n it should just be a matter of changing the text in the locale file and changing the construction of the flash here to not be an array (looks like we usually do multiline flashes with <br />)? If yes, you have my official AD&T blessing to adopt AO3-6765
3 participants
Pull Request Checklist
as the first thing in your pull request title (e.g.
AO3-1234 Fix thing)until they are reviewed and merged before creating new pull requests.
Issue
https://otwarchive.atlassian.net/browse/AO3-7397
Purpose
Redirect to the URL specified in the config with
SKIN_PREVIEW_URLwhen previewing a skin.Remove the functionality to redirect to the works index of a random tag - this provided no guarantees on the kind of content shown and would sometimes choose tags that no longer existed (resulting in 500 errors).
Credit
nicolacleary (she/her)