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

Feature/ins-1518-improve-create-request-dropdownmodal #4812

Conversation

jackkav
Copy link
Contributor

@jackkav jackkav commented May 23, 2022

Request shortcuts in new request/folder dropdown. It was hard to find gRPC when it was nestled in the http methods dropdown. This aims to raise the discoverability of gRPC, GraphQL and in future websockets.

Changes

  • New request modal previously showed mime type selection on post put and patch hiding it for other methods, this isn't suitable for graphql. Therefore I have elected to remove the hiding and showing of the mime type selection in order to simplify the use cases of this form.

Friction

  • the existing mime type show/hide UX is too twitchy, there is only one reasonable case for it to be disabled/hidden which is grpc, however I would argue this is still the wrong place for GRPC to go since the other options are all HTTP methods. This logic holds true for websockets too. Therefore I have gone with the approach above.
  • there's a lot of prop drilling and indirection in this code path, related to hotkeys.
  • open to suggestions about default hotkeys for grpc and graphql...
  • the ticket INS-1518 suggests removing the New Request modal flow entirely but that would conflate insomnia approach to request naming, that could be scoped out of this PR and still deliver on increased discoverability. However removing modal code could be a "good thing ™" since we have issues with the implementation of modals.

New Dropdown
image

Old Modal
image

Update: removed new request modal
In light of this change a few new issues arise.
Friction

  • new folder flow is now inconsistent and has different rename flow.
  • grpc still has a set proto modal, which could be delayed until the url is filled out.
  • quick create request hotkey is now identical to create request, so I've renamed it to create http request.
  • should folder context menu also contain these quick request types? Not yet.
  • should I transform the two dropdown components to function components? would entail untangling the dropdown hide show ref drilling.

changelog(Improvements): Improved the create new request shortcuts dropdown

@jackkav jackkav self-assigned this May 24, 2022
@jackkav jackkav force-pushed the feature/ins-1518-improve-create-request-dropdownmodal branch 4 times, most recently from c38c5e1 to f06e648 Compare May 24, 2022 19:38
@jackkav jackkav force-pushed the feature/ins-1518-improve-create-request-dropdownmodal branch from 65762bb to eab9215 Compare May 25, 2022 08:42
@jackkav jackkav marked this pull request as ready for review May 25, 2022 13:46
Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

hey: reminder that we want analytics events for stuff like this

Copy link
Contributor

@gatzjames gatzjames left a comment

Choose a reason for hiding this comment

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

Seems that most of the friction points have been moved to new issues 👏
Imo migrating the dropdown seems like a big task for this PR so we could keep it out of scope for now.
Tested and it works great! 🚀

@jackkav jackkav force-pushed the feature/ins-1518-improve-create-request-dropdownmodal branch from 9e861af to 07a9e33 Compare June 1, 2022 13:07
@jackkav
Copy link
Contributor Author

jackkav commented Jun 1, 2022

Tested analytics from new request in folder, add button and empty state. All send successfully @dimitropoulos

Copy link
Contributor

@dimitropoulos dimitropoulos left a comment

Choose a reason for hiding this comment

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

I know I know we're trying to get this PR over the line and I hate having to ask this.. but what's the story with the request group menu not having the graphql and grpc items anymore in the last commit? Apologies if I missed it somewhere.


Also, independent of that, the change to put unbound functions in callbacks (e.g. onClick={() => someThing()}}) has pretty sever performance problems it presents (over time, especially).


I did, however, confirm that the segment events area all working now, so that's good at least.

@jackkav
Copy link
Contributor Author

jackkav commented Jun 1, 2022

It wasn't in the ticket, so I thought to add it if it was straightforward. However once I did the number of new decisions to make increased the scope too much for a single PR.

@jackkav jackkav force-pushed the feature/ins-1518-improve-create-request-dropdownmodal branch from 83f3645 to ff0a7ce Compare June 1, 2022 22:26
@jackkav jackkav merged commit c49111b into Kong:develop Jun 2, 2022
@jackkav jackkav deleted the feature/ins-1518-improve-create-request-dropdownmodal branch June 2, 2022 07:31
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.

5 participants