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

feat(front): improve bin deletion UX #157

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Mesteery
Copy link
Collaborator

The bin deletion via the interface was completly impratical.

From now on, the token storage is done automatically.
Indeed, a Service Worker is registered to intercept in particular
the request of the form and the request of the final page (redirection).
This allows to associate the token with the bin ID, which is not known
in advance.

In the background, the IndexedDB database is used via the localforage
abstraction, which allows to easily store key-value in the same way as
the localStorage (which is not available in a Service Worker).

This feature is only available for the clients with JavaScript enabled.

Closes: #147.

@Julien00859 Julien00859 self-requested a review November 28, 2021 14:50
Copy link
Member

@Julien00859 Julien00859 left a comment

Choose a reason for hiding this comment

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

Frankly I don't know, like a service worker is something I consider huge, I really don't feel easy giving my approval on this one, nor I feel easy rejecting the changes because it do works and it's a feature that have long been requested by users.

My only comment it that it definitely deserve more effort documentation-wise. Maybe add a link to online resources we can study to understand how it works. Also still documentation-wise, it would be great you explain all our attempts to make this one work and to re-state the "user-story" of this branch.

I delegate my review to someone who knows more than I do on this piece of technology.

@Julien00859
Copy link
Member

When you'll resolve the conflict, could you add a comment for the noscript tag explaining that the feature requires javascript and that this bit of code hide the menu in no-script environments ?

@Mesteery
Copy link
Collaborator Author

When you'll resolve the conflict, could you add a comment for the noscript tag explaining that the feature requires javascript and that this bit of code hide the menu in no-script environments ?

Done

@Julien00859
Copy link
Member

Still waiting for a third person to review and test this branch!

@Mesteery
Copy link
Collaborator Author

cc @fusetim or anyone who wants to test this.

@fusetim
Copy link
Member

fusetim commented Nov 29, 2021

Might take a while for me

@Julien00859 Julien00859 mentioned this pull request Nov 29, 2021
@Mesteery
Copy link
Collaborator Author

bump @Julien00859?

@AntoineJT AntoineJT self-requested a review September 11, 2022 17:02
@AntoineJT
Copy link
Member

De mon côté le bouton de suppression de bin ne s'affiche pas
Ai-je mal fait quelque chose ? @Mesteery

Copy link
Member

@AntoineJT AntoineJT left a comment

Choose a reason for hiding this comment

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

Après correction des deux commentaires dans ma review, la fonctionnalité ne fonctionne toujours pas de mon côté, il faut que tu y jettes un oeil

@@ -6,7 +6,10 @@
<link rel=stylesheet href="/assets/styles/monokai.css">
<script src="/assets/scripts/loc.js" defer></script>
<script src="/assets/scripts/report.js" defer></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/localforage/1.10.0/localforage.min.js" defer></script>
<script src="/assets/scripts/delete.js" defer type=module></script>
Copy link
Member

Choose a reason for hiding this comment

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

Le type module n'est pas nécessaire et empêche le chargement a cause d'un type mime invalide
D'ailleurs les autres scripts sont chargés avec un type mime invalide aussi, mais ça ne provoque qu'un warn tant que ce n'est pas chargé en tant que module

const deleteButton = document.getElementById('delete-button');
const id = location.pathname.slice(1, location.pathname.lastIndexOf('.'));

const token = await localforage.getItem(id);
Copy link
Member

Choose a reason for hiding this comment

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

un await en dehors d'une fonction async empêche l'exécution du script

The bin deletion via the interface was completly impratical.

From now on, the token storage is done automatically.
Indeed, a Service Worker is registered to intercept in particular
the request of the form and the request of the final page (redirection).
This allows to associate the token with the bin ID, which is not known
in advance.

In the background, the IndexedDB database is used via the `localforage`
abstraction, which allows to easily store key-value in the same way as
the `localStorage` (which is not available in a Service Worker).

This feature is only available for the clients with JavaScript enabled.

Closes: readthedocs-fr#147.
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.

front: meilleur UX pour supprimer ses bins
4 participants