Improve preview feedback in fetch query

[bytasv]

• Closes Show undefined results in fetch preview window #1083

Added logic for:

• Checking if we have any data returned but potentially not mapped correctly with transform function
• If preview is invoked and user is looking at different tab, transform tab will become active
• If request completes but returns empty response - show appropriate message

[render]

Your Render PR Server URL is https://toolpad-pr-1153.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-cd42241gp3jra0rsd3jg.

[Janpot]

• If preview is invoked and user is looking at different tab, transform tab will become active

I don't feel like this is the right approach. We should leave the user in control of navigating the UI. Perhaps we can make the word "transform" in the warning clickable to open the tab?

[bytasv]

I don't feel like this is the right approach

I personally feel differently about this. Can you provide an example where you wouldn't want to check how transform is defined after getting this message?
We could also do a link, but IMO this is a better UX (at least from my perspective)

-edit: I'd consider it a very similar pattern to when you get error in a long form and browser scrolls to the actual error so you are not left hanging wondering what's wrong

[Janpot]

Can you provide an example where you wouldn't want to check how transform is defined after getting this message?

It's not about interested in it or not. It's about not disrupting the UI without user interaction. Maybe the backend takes 7 seconds to respond and in those 7 seconds I may have started to look in one of the other tabs. Suddenly out of nothing the tab changes and I'm pulled out of my context. Instead, in the preview message give a button to open the relevant tab so that I can correct it in a time that makes more sense for me as a user (e.g. when I'm inspecting the message in the preview window). Another option is to mark the tab with a badge to tell the user there is some important action to be taken there.

edit:

there's an important difference with scrolling to a validation error. it's that in that case the scroll happens when the user clicks the button. In the case of the query preview it happens async, potentially a long while after clicking the button

[bytasv]

there's an important difference with scrolling to a validation error. it's that in that case the scroll happens when the user clicks the button. In the case of the query preview it happens async, potentially a long while after clicking the button

Not necessarily, it can also happen same way as you described with preview - you submit, data is validated async on API, API is slow, you get response after same 7 seconds.
IMO situation about pressing preview and immediately starting to edit something is more hypothetical, than actually waiting to see what's the result.
Anyways, lets see what others think and I will change it if needed

[Janpot]

Not necessarily, it can also happen same way as you described with preview - you submit, data is validated async on API, API is slow, you get response after same 7 seconds.

I'd consider that a flawed UX. I think a user should stay in control of the scroll position. If the validation comes back async I'd consider showing a notification "x validation errors" that scrolls to the relevant field on click.

One of the main tenets of user interface design, is that the user needs to feel in control, and mostly, that they are in control of their time. if we're not going to block the whole UI while preview is loading, then we shouldn't manipulate it without user action.

IMO situation about pressing preview and immediately starting to edit something is more hypothetical

Nothing hypothetical about that, I do it all the time. e.g. I often realize right when I click the button that I forgot something. It's also not only editing, I may just be reading through the other tabs to see which options are available.

[bytasv]

cc @gerdadesign for your input about above ☝️

[Janpot]

Just to note that the specific feature in question is not strictly necessary to close the ticket. Another option could be to merge without it and take our time to define the right UX. (e.g. in a follow-up issue, or RFC,...)

[Janpot]

Another option could be to merge without it

@bytasv I might have not expressed myself very well, so the idea was be to merge this PR without the automatic tab switching. Or perhaps this was merged accidentally?

[Janpot]

I don't think the logic is sound here. It should be perfectly valid to return empty arrays or empty objects from the transform function. Similarly, untransformedData can perfectly return an empty array while the transformed data returns undefined.

[bytasv]

Sorry it's not really clear what you are proposing or what's exactly the issue?

Similarly, untransformedData can perfectly return an empty array while the transformed data returns undefined.

Can you provide an example?

[Janpot]

Can you provide an example?

Sure thing, here's a transform function that legitimately returns an empty array:

use url: https://httpbin.org/json
use transform: return data.slideshow.slides.filter(slide => slide.title.includes('foo'));

I believe this should not show the warning.

I also think we should accept null as a legitimate value and only show a warning when the transform is returning undefined.
We should probably also make the JsonView show undefined instead of nothing when the input is undefined

Lastly, I believe we should also consider preventing the overuse of basic lodash methods through no-restricted-imports and only whitelist a few like pick, throttle, get and set. Methods like isEmpty cover a too broad range of values and result in bugs like these. Also, the word "empty" doesn't have a well defined meaning in programming, making the code less intuitive. Better to use native constructs to express the intent clearer to other maintainers

[bytasv]

Thanks, that's great feedback, I've addressed it in a follow up PR: #1165

[bytasv]

@bytasv I might have not expressed myself very well, so the idea was be to merge this PR without the automatic tab switching. Or perhaps this was merged accidentally?

No, I did set this to merge as that's how I understood your comment. As for feature itself, I disagree with you, I think tab switching is a good UX in this case, just as I explained in previous comment, so I will not revert this change for now. If there is more feedback from others adding that it should not behave like that we can change it.

[apedroferreira]

About switching tabs automatically, I also don't like that type of pattern as it can be disruptive or annoying to users.
In my opinion we should leave it up to the user, I think there are more cons than pros to it.