studio icon indicating copy to clipboard operation
studio copied to clipboard

fix: ask user for reload and use url in parsing

Open magicmatatjahu opened this issue 2 years ago • 20 comments

Description

  • ask use for reload when spec is modified - https://github.com/asyncapi/studio/issues/393
  • when url is passed in query parser uses url as path - https://github.com/asyncapi/studio/issues/392
  • when url is passed in query, user see two buttons in editor sidebar: save to storage and load from storage which one saves content from editor to the storage and switches to the localStorage mode and second one loads content from localStorage and also switches to the localStorage mode.

I have some problems (or rather concerns) related to the UX:

  • when user pass in query url parameter and when click on one of two buttons described above, studio doesn't remove that url parameter from href so if user will reload studio, url "logic" will be still here - should we remove url?

Any UX feedback is more than welcome :)

You can test it by passing url parametr like in example:

https://deploy-preview-394--modest-rosalind-098b67.netlify.app/?url=https://raw.githubusercontent.com/asyncapi/spec/master/examples/social-media/public-api/asyncapi.yaml

cc @derberg @mcturco

Related issue(s) Resolves #393 Resolves #392

magicmatatjahu avatar Jul 25 '22 13:07 magicmatatjahu

Deploy Preview for modest-rosalind-098b67 ready!

Name Link
Latest commit 6eef8fc17a4a7752d79141a7f2f01c90c96eee1e
Latest deploy log https://app.netlify.com/sites/modest-rosalind-098b67/deploys/62e3a3de2e1c800009e9631e
Deploy Preview https://deploy-preview-394--modest-rosalind-098b67.netlify.app/
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Jul 25 '22 13:07 netlify[bot]

ask use for reload when spec is modified - https://github.com/asyncapi/studio/issues/393

but spec is saved in local storage already, I guess on every new button release we save it and then on reload load from local storage?

better would be to enable saving of multiple AsyncAPI files in local storage, by name, and then loading by name 😉

☝🏼 this is the only reason why I would see save to storage and load from storage making sense. It is cool that now on import from URL you display that it was imported. And I would like to see save to localStorage button only if I can tag the file by a name I can later use with Import from LocalStorage.

When Importing from URL, the moment I start editing the file, label should directly change to From localStorage and be saved there without clicking

derberg avatar Jul 25 '22 14:07 derberg

but spec is saved in local storage already, I guess on every new button release we save it and then on reload load from local storage?

Hmm, I don't understand 😅 Browser will ask user for reload only when spec (in editor) is modified, so if you have "url" mode and if you modify editor's content you will see prompt in reloading/closing tab. Similar situation with localStorage mode.

better would be to enable saving of multiple AsyncAPI files in local storage, by name, and then loading by name 😉

Would be, but not in this PR 😄

It is cool that now on import from URL you display that it was imported

It always indicates that you use file from url - you can check it https://studio.asyncapi.com/?url=https://raw.githubusercontent.com/asyncapi/spec/master/examples/social-media/public-api/asyncapi.yaml

When Importing from URL, the moment I start editing the file, label should directly change to From localStorage and be saved there without clicking

It's working in this way now, in deployed latest studio, but please check discussion in this issue for more details. Some users have problem with that. https://github.com/asyncapi/studio/issues/393

magicmatatjahu avatar Jul 25 '22 14:07 magicmatatjahu

UX related I'd say:

  1. The two buttons (Save in storage and Load from storage) are simple and effective.
    1. I feel like I'm in control whether whatever is in my localStorage remains there, or I explicitly want to overwrite with what has been loaded from that url.
    2. Tested the behavior and it works just fine: my local stuff gets saved and I don't lose it if I'm loading something (using this url feature).
  2. Once the User clicks Save in storage, I'd remove the url query param since it doesn't reflect the context anymore.

Regarding having multiple files locally, that could be an addition, sure, although I wouldn't stress that much with it. Maybe a polling in the community would help to see the need. To me, atm, I don't feel I'd need it.

@magicmatatjahu Thanks, Maciej, for doing this! It's really helpful.

dxps avatar Jul 25 '22 15:07 dxps

Oh, and one more thing: I've notice that I'm being asked to confirm that I want to leave the page. But only when I'm opening Studio with an url query param.

That's fine, except that it always asks for this, whether I changed or not the loaded content. Again, UX related (to minimize the friction), I'd expect to be asked for this confirmation only if I changed the content (and not saved it to the localStorage, ofc).

dxps avatar Jul 25 '22 15:07 dxps

Hi! I'm here for UX feedback :)

I agree that the two buttons are simple and effective but to me it only makes sense after I have read this PR with the context. I am wondering if for everyone else that it would be clear enough what the actions mean from those two buttons or if the user may miss it too easily?

Might I suggest that instead of adding these buttons/actions in the sidebar that we add them in the Popover component so it would be more visible to the user? I'm thinking of verbiage along the lines of:

# You are viewing a document from another URL
Would you like to replace your last session with the contents in this document? This will erase any changes from your last session.

[Restore Previous Session] | [Update Session to Current Document]

Might need some more UX Writing thought, but this is a start to making the tool more "human-friendly" 😄


Once the User clicks Save in storage, I'd remove the url query param since it doesn't reflect the context anymore.

Agreed!

mcturco avatar Jul 25 '22 18:07 mcturco

@dxps Thanks!

Oh, and one more thing: I've notice that I'm being asked to confirm that I want to leave the page. But only when I'm opening Studio with an url query param.

Hmm. Is some cases it doesn't ask in reload but in some asks, I will check that. You probably have enabled autosave so when you are in localStorage mode then content is updated automatically so due to this fact you don't see popup in closing/reloading browser tab. I hope you understand that "feature". You should disable autosaving and then it works as should.

Once the User clicks Save in storage, I'd remove the url query param since it doesn't reflect the context anymore.

Ok, I will remove it :)

I agree that the two buttons are simple and effective but to me it only makes sense after I have read this PR with the context. I am wondering if for everyone else that it would be clear enough what the actions mean from those two buttons or if the user may miss it too easily?

Might I suggest that instead of adding these buttons/actions in the sidebar that we add them in the Popover component so it would be more visible to the user? I'm thinking of verbiage along the lines of:

@mcturco Yeah, I understand that problem, but could you precise where that popover component should be showed? Like the survey popover, in the bottom left corner or where? Or do you mean that popover component in the top right corner of editor, where you open editor menu?

magicmatatjahu avatar Jul 27 '22 10:07 magicmatatjahu

@dxps Ok, now it should be fine:

  • url is removed when you load or save to storage content from url
  • ask popup shows when you change something in the editor's content - if it's content from url, it compares the content from url and what you have in the editor, which means that if you changed something in the editor, it will ask you about the change. When it comes to localStorage, as I described in the previous comment - it will ask you only if the content is changed in relation to localStorage - if you have auto saving then it will never ask you.

I hope it's fine :)

magicmatatjahu avatar Jul 27 '22 12:07 magicmatatjahu

@magicmatatjahu Good stuff, Maciej! :man_dancing:

  1. Just tested using the same deploy-preview with an url, and it's legit:

    1. Without changing the content, I don't get asked for a confirmation to leave the page.
    2. Clicking Load from storage or Save in storage it correctly loads from or saves to, respectively. In both cases, url query param is removed.
  2. Previously, I was referring to the fact that I was asked to confirm that I want to leave the page, only when when opening Studio with the url query param, but without even touching the content. Of course, no change was performed. But anyway, currently it seems that this has been solved (according with 1.i mentioned before).

dxps avatar Jul 27 '22 15:07 dxps

@mcturco Yeah, I understand that problem, but could you precise where that popover component should be showed? Like the survey popover, in the bottom left corner or where? Or do you mean that popover component in the top right corner of editor, where you open editor menu?

Hmmmm, my first instinct says to put it in the bottom left like we had for the survey, but do you think that would be noticeable enough that the user knows to take action? If we think, "no" to that question, my second thought is to put it just over the document as shown in this screenshot:

Screen Shot 2022-07-28 at 9 26 08 AM

What do you think?

mcturco avatar Jul 28 '22 13:07 mcturco

@mcturco Thanks for response!

Hmmmm, my first instinct says to put it in the bottom left like we had for the survey, but do you think that would be noticeable enough that the user knows to take action? If we think, "no" to that question, my second thought is to put it just over the document as shown in this screenshot:

Is there a point to having this popover? It seems to me that when someone loads something from an url they would want to be able to edit the url' spec in editor without overwriting the spec from storage. Should this popover appear when hovering over something or should it always be visible?

magicmatatjahu avatar Jul 28 '22 13:07 magicmatatjahu

Is there a point to having this popover? It seems to me that when someone loads something from an url they would want to be able to edit the url' spec in editor without overwriting the spec from storage. Should this popover appear when hovering over something or should it always be visible?

True, probably not the best idea to cover the spec. I guess the goal in my mind is to create an obvious "notification" that there is action required by the user on whether they want to save this new spec to local or keep what they already have and some brief explanation on what would happen if they saved the new URL data to storage.

To me, right now this notice is not super obvious, and it lacks a little bit of "warning" or context as to what would happen if the user decided to click the "save to storage" button, which could result in data loss.

mcturco avatar Jul 28 '22 14:07 mcturco

@mcturco

To me, right now this notice is not super obvious, and it lacks a little bit of "warning" or context as to what would happen if the user decided to click the "save to storage" button, which could result in data loss.

Would a confirmation modal be ok?

magicmatatjahu avatar Jul 28 '22 15:07 magicmatatjahu

Would a confirmation modal be ok?

Yeah that was the next thing I was thinking. Like we dull out the background and ask the user to make a selection before proceeding. But then, we get the same issue where the user can't first preview the spec file before deciding whether or not to save it. That's why I think the popover would be the best solution

mcturco avatar Jul 28 '22 15:07 mcturco

@mcturco

Yeah that was the next thing I was thinking. Like we dull out the background and ask the user to make a selection before proceeding. But then, we get the same issue where the user can't first preview the spec file before deciding whether or not to save it. That's why I think the popover would be the best solution

No, no! That modal will occur only when user click button for save or load :)

magicmatatjahu avatar Jul 28 '22 15:07 magicmatatjahu

Or probably best option will be to render tooltip with description what exactly save or load makes 😄

magicmatatjahu avatar Jul 28 '22 15:07 magicmatatjahu

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

sonarqubecloud[bot] avatar Jul 29 '22 09:07 sonarqubecloud[bot]

@mcturco Ok, I added native modals for these two actions:

image

image

I have not had much time, but I will improve it when I return from vacation. What do you think about it?

magicmatatjahu avatar Jul 29 '22 10:07 magicmatatjahu

Hey @magicmatatjahu I think this is a good implementation for the meantime, but would love to have a follow-up issue where we put the warnings in a modal instead of an alert window. Let me know if you want me to raise that issue!

mcturco avatar Jul 29 '22 13:07 mcturco

@mcturco https://github.com/asyncapi/studio/issues/402 :)

magicmatatjahu avatar Jul 29 '22 13:07 magicmatatjahu

This pull request has been automatically marked as stale because it has not had recent activity :sleeping:

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience :heart:

github-actions[bot] avatar Nov 27 '22 00:11 github-actions[bot]

This pull request has been automatically marked as stale because it has not had recent activity :sleeping:

It will be closed in 120 days if no further activity occurs. To unstale this pull request, add a comment with detailed explanation.

There can be many reasons why some specific pull request has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this pull request forward. Connect with us through one of many communication channels we established here.

Thank you for your patience :heart:

github-actions[bot] avatar Mar 29 '23 00:03 github-actions[bot]