Closed Parthiv-M closed 1 month ago
Hi @rugk, would be great if you can take a look at this patch :)
With these two comments resolved, my only concern remaining is the UX one - forcing a reload without user interaction might hide the message before they see it. For this reason I prefer the current behaviour where the user has to explicitly click on the "New" button to move on to create a new paste (if that is what they want - maybe they rather take a screenshot as proof, or just close the browser tab).
Let's give the other maintainers a chance to speak up on this. If no one is opposed though, I'm willing to merge it, after updating the SRI-hashes and documenting the change in the changelog.
I have a suggestion from the UX perspective that could probably be better than the current method of clicking on the PrivateBin logo to go back to the home page: inside the green alert itself, we place a button that says something like <-- Back home
or <-- Create new paste
with the <--
being replaced by a icon.
Happy to hear feedback from the maintainers about this :)
[...] inside the green alert itself, we place a button that says something like
<-- Back home
or<-- Create new paste
with the<--
being replaced by a icon.
That would be a lot better, IMHO. It could be a button with icon, maybe styled just like the new button? For example, in the bootstrap5.php template:
<div id="status" role="alert" class="alert alert-<?php echo ($STATUS == 'Paste was properly deleted.' ? 'success' : 'info'), (empty($STATUS) ? ' hidden' : '') ?>">
<svg width="16" height="16" fill="currentColor" aria-hidden="true"><use href="img/bootstrap-icons.svg#info-circle" /></svg>
<?php echo I18n::encode($STATUS), PHP_EOL; ?>
</div>
<?php
if ($STATUS == 'Paste was properly deleted.') :
?>
<button type="button" class="btn btn-secondary flex-fill" onclick="$.PrivateBin.UiHelper.reloadHome()">
<svg width="16" height="16" fill="currentColor" aria-hidden="true"><use href="img/bootstrap-icons.svg#file-earmark" /></svg> <?php echo I18n::_('New'), PHP_EOL; ?>
</button>
<?php
endif;
?>
Aside: I realize now that this will only work for English, as the $STATUS contains an already translated message. We may have to detect this around the below line in the controller and assign some other, boolean flag, say isDeleted
to the view that we can more easily check in the above logic:
https://github.com/PrivateBin/PrivateBin/blob/d69d29f3a940b443760f1e227df6628e3f4cfb3f/lib/Controller.php#L414
Ah, makes sense! Let me try to make these changes and update this patch
So, I've made the following changes:
onclick
in the template would cause issues with loading the jquery function (due to CSP config)The button with Bootstrap 5
The button with Bootstrap 3 has a few options, I've pushed the one that looked best to me, but it is up for suggestions/changes :)
btn-default
(pushed)
btn-primary
btn-secondary
Did not you want to name it differently than "New"? "Back"? Without context, at this position, it is quite unclear IHMO, what "new" thing can be created here?
Ideas:
Did not you want to name it differently than "New"?
I want to go for New Paste, I'll make that change
Additionally, is there something I need to do to update the SRIs? And the changelog? If you can give me links to some docs, I can do that too.
Better than "New" in any case, though I liked the other options a bit more (with changed icons like a reload one or so of course).
About SRI: see https://github.com/PrivateBin/PrivateBin/wiki/Development#subresource-integrity-for-javascript-resources please
And the Changelog yeah, you can add an entry there if you feel like doing so.
Hi @rugk, I've made the following changes
CHANGELOG.md
fileI had one query (which is also why one test is failing in the CI): do I have to add keys and values for all the languages (every <lang-code>.json
file)? If so, I can do that (and push the SRI fix) and everything should be done.
This PR fixes #1156
Changes
alert-success
(green coloured) if$STATUS
equals thePaste was properly deleted.
message;alert-info
otherwise