AO3-6765 Improve Skin Preview Message #4934
Conversation
BoldestDungeon
force-pushed
the
AO3-6765__improveSkinPreviewMessage
branch
2 times, most recently
from
d501414 to
b83403c
Compare
* Adds the skin ID to the preview tip (rather than making the user copy-paste it themselves) * Wraps the "Return" button in a paragraph tag so that it is not a bulleted list item * Adjusts wording in message
BoldestDungeon
force-pushed
the
AO3-6765__improveSkinPreviewMessage
branch
from
b83403c to
43bd263
Compare
| flash[:notice] << "<a href='#{skin_path(@skin)}' class='action' role='button'>".html_safe + ts("Return To Skin To Use") + "</a>".html_safe | ||
| flash[:notice] << ts("Go back or follow any link to remove the skin.") | ||
| flash[:notice] << (ts("Tip: You can preview any archive page you want by adding '?site_skin=%{skin_id}' to the end of the URL.", skin_id: @skin.id) + | ||
| "<p><a href='#{skin_path(@skin)}' class='action'>".html_safe + ts("Return to Skin to Use") + "</a></p>".html_safe) |
There was a problem hiding this comment.
Apparently the linter had a field day with this file when I saved it - the three lines above are the only three deliberately edited in this file, the rest was the auto-linting, sorry.
| When I follow "Return To Skin To Use" | ||
| And I should see "Go back or follow any link to remove the skin" | ||
| And I should see "Tip: You can preview any archive page you want by adding '?site_skin=" | ||
| And I should see "' to the end of the URL." |
There was a problem hiding this comment.
I'm not familiar enough with Ruby test suites to have been able to figure out how to put the actual test-skin ID into this string, so split it up into the before and after parts. If there's a better way to do that I would love to learn :)
There was a problem hiding this comment.
I think this is fine; if we wanted to check the actual ID, we would need to add a custom step to https://github.com/otwcode/otwarchive/blob/master/features/step_definitions/skin_steps.rb (which IMO is overkill here)
brianjaustin
changed the title
| if current_user && current_user.is_a?(User) | ||
| @preference = current_user.preference | ||
| end | ||
| @preference = current_user.preference if current_user && current_user.is_a?(User) |
There was a problem hiding this comment.
One better over Rubocop, we don't need to check nil at all here because nil.is_a?(User) will return false. So, this can be simplified down to
| @preference = current_user.preference if current_user && current_user.is_a?(User) | |
| @preference = current_user.preference if current_user.is_a?(User) |
| if @user != current_user | ||
| flash[:error] = "You can only browse your own skins and approved public skins." | ||
| redirect_to skins_path and return | ||
| end | ||
| if is_work_skin | ||
| @skins = @user.work_skins.sort_by_recent | ||
| @title = ts('My Work Skins') | ||
| @title = ts("My Work Skins") |
There was a problem hiding this comment.
Since we're cleaning up the auto-fixable Rubocop issues in this file already, could you update these uses of ts to us the Rails translation helper instead?
| flash[:notice] << (ts("Tip: You can preview any archive page you want by adding '?site_skin=%{skin_id}' to the end of the URL.", skin_id: @skin.id) + | ||
| "<p><a href='#{skin_path(@skin)}' class='action'>".html_safe + ts("Return to Skin to Use") + "</a></p>".html_safe) |
There was a problem hiding this comment.
Same thing about the translation helper, but also noting that this does not follow our current internationalisation standards. Generally, we'd now prefer something like
| flash[:notice] << (ts("Tip: You can preview any archive page you want by adding '?site_skin=%{skin_id}' to the end of the URL.", skin_id: @skin.id) + | |
| "<p><a href='#{skin_path(@skin)}' class='action'>".html_safe + ts("Return to Skin to Use") + "</a></p>".html_safe) | |
| flash[:notice] << t(".preview_tip_html", | |
| skin_id: @skin.id, | |
| return_to_skin_link: tag.p(link_to(t(".return_to_skin"), skin_path(@skin)))) |
with the corresponding keys in https://github.com/otwcode/otwarchive/blob/master/config/locales/controllers/en.yml. (I haven't run the above through a linter, so some tweaks will probably be needed for it to work perfectly.)
Additionally, make sure to swap out the single quotes around 'site_skin?=%{skin_id}' to be double quotes "site_skin?=%{skin_id}" (this should require less escaping when things are moved into the YAML file, so I'd recommend doing that move first).
| When I follow "Return To Skin To Use" | ||
| And I should see "Go back or follow any link to remove the skin" | ||
| And I should see "Tip: You can preview any archive page you want by adding '?site_skin=" | ||
| And I should see "' to the end of the URL." |
There was a problem hiding this comment.
I think this is fine; if we wanted to check the actual ID, we would need to add a custom step to https://github.com/otwcode/otwarchive/blob/master/features/step_definitions/skin_steps.rb (which IMO is overkill here)
Issue
https://otwarchive.atlassian.net/browse/AO3-6765
Purpose
Testing Instructions
Updated preview message following the changes in the ticket should look like this:
References
Are there other relevant issues/pull requests/mailing list discussions?
Credit
David Bilsky/Ironskink (He/Him)