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: introduce request group settings (name, description, and move/copy) #3350

Merged
merged 12 commits into from
Jun 25, 2021

Conversation

NickHackman
Copy link
Contributor

@NickHackman NickHackman commented May 2, 2021

Adds the ability to modify the description for a Request Group in a modal shown by hitting settings in a dropdown. Identical implementation to RequestSettings. In addition allows renaming the Request Group as well as moving/copying it to a different workspace.

closes: #3306

Screenshots

Screen Shot 2021-05-02 at 6 48 11 PM

Following pattern set by Requests as this goes after plugins.

Screen Shot 2021-05-02 at 6 48 34 PM

The words "Request Group" here feels wordy personally, I prefer "Folder", but whatever you think is best. This also adds the support of copying/moving workspaces as it is almost identical to the Request implementation.

I think opening the docs (in the request view/area) and expanding the folder/group is an obvious and solid approach. It would also have the benefit of being easily expanded upon, allowing in the future to have Folder Plugins as buttons on the far right side of where requests have headers/authorization or other group/folder features (ie: authorization, headers, etc). Anyways, just like to ensure that I'm on the same page here.

My comment here, I feel like this should be a different issue, but I'm down to address it if you'd like, I personally really want some of those future features as being able to add authorization headers for ALL requests in a group would be AWESOME!

I appreciate all feedback, thanks! 👍

Adds the ability to modify the description for a Request Group in a modal shown by hitting settings in a dropdown. Identical implementation to RequestSettings. In addition allows renaming the Request Group as well as moving/copying it to a different workspace.

Ref: Kong#3306
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, and welcome to the project! We currently have a code-freeze in place because we're transitioning to typescript (1:1, should be fairly simple to update this PR), so I won't merge or test this just yet. 🤗

The words "Request Group" here feels wordy personally, I prefer "Folder", but whatever you think is best.

Let's go with folder! RequestGroup is just the verbiage used in the code but at some point in the past the UI started to use Folder.

This also adds the support of copying/moving workspaces as it is almost identical to the Request implementation.

That seems fine to me because we get it for free by deriving the implementation from RequestSettingsModal. It would be worth checking there is nothing special happening in the "Move" dropdown item here. If so, let's bring that over and remove this entry.
image

I think opening the docs (in the request view/area) and expanding the folder/group is an obvious and solid approach. It would also have the benefit of being easily expanded upon, allowing in the future to have Folder Plugins as buttons on the far right side of where requests have headers/authorization or other group/folder features (ie: authorization, headers, etc). Anyways, just like to ensure that I'm on the same page here.

My comment here, I feel like this should be a different issue, but I'm down to address it if you'd like, I personally really want some of those future features as being able to add authorization headers for ALL requests in a group would be AWESOME!

Yep these things would be separate issues for sure. Having workspace/folder defaults have been requested in the past. It's not in our internal roadmap yet (AFAIK) but if you want to solutions in respective issues I can get feedback from our designer about those. 😄

No longer need Request Group Move as it is present in Settings. In addition, fix the implementation in Settings to be exactly the same as the implementation present in move-request-group-modal which is duplicate then remove current rather than solely an update.
@NickHackman
Copy link
Contributor Author

We currently have a code-freeze in place because we're transitioning to typescript (1:1, should be fairly simple to update this PR), so I won't merge or test this just yet.

Awesome, Typescript sounds like a good choice for a project this big.

Let's go with folder!

Done as of 9e642d1!

That seems fine to me because we get it for free by deriving the implementation from RequestSettingsModal. It would be worth checking there is nothing special happening in the "Move" dropdown item here. If so, let's bring that over and remove this entry.

Removed and made necessary changes to have the same implementation as the modal that was used prior in 90a6aa5. Weirdly the previous implementation was a duplicate followed by a remove, not entirely sure how that's different than an update, but if it works, it works 😆

Yep these things would be separate issues for sure. Having workspace/folder defaults have been requested in the past. It's not in our internal roadmap yet (AFAIK) but if you want to solutions in respective issues I can get feedback from our designer about those. 😄

Good to hear, I'll have some up in the near future 🙂

@develohpanda develohpanda changed the title feat: request group descriptions feat: introduce request group settings (name, description, and move/copy) May 4, 2021
@develohpanda
Copy link
Contributor

Weirdly the previous implementation was a duplicate followed by a remove, not entirely sure how that's different than an update, but if it works, it works 😆

That checks out! The duplicate and remove db functions will operate on all children of the entity (request-group in this case). The update will only operate on the entity itself.

@NickHackman NickHackman force-pushed the request-group-descriptions branch from 4bef4d9 to ee54114 Compare June 18, 2021 20:45
@NickHackman
Copy link
Contributor Author

NickHackman commented Jun 18, 2021

@develohpanda fixed merge conflicts and ported new file to Typescript, should be ready for review again 👍

@NickHackman
Copy link
Contributor Author

@develohpanda any idea why the CI Windows Test is failing? Looks like a permission issue (line) seems unrelated, but I don't know how others are passing and this has failed twice. Previous instance failed due to a connection reset (line). 🤷‍♂️

@develohpanda
Copy link
Contributor

@NickHackman ah, windows has been a bit buggy recently and failing on random issues like that. I'll kick it off again!

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

LGTM, looks to be working well! We have some improvements we want to make in the future with modals (now that we are in TS) but until that's documented and the existing modals are updated, what you've done here is perfect. 🙌🏽

image

@develohpanda develohpanda requested review from wdawson and removed request for reynolek June 21, 2021 10:17
@wdawson
Copy link
Contributor

wdawson commented Jun 21, 2021

Looks great to me too. Thanks, @NickHackman for the PR! ⛵

Also, hi! I'm the new PM on the Insomnia team 👋

@NickHackman
Copy link
Contributor Author

Thanks for all your help @develohpanda, appreciate it and I'll for sure be contributing more in the future.

@wdawson That's awesome! I have some other issues open that I'd love your opinion on 😉 (#3368, #3351)

Looks like Windows is failing again due to connection reset, if you don't mind @develohpanda restarting the build again

@develohpanda
Copy link
Contributor

Just FYI @NickHackman waiting on one more eng review before merging 😄 almost good to go

@vercel vercel bot temporarily deployed to Preview June 23, 2021 19:23 Inactive
@dimitropoulos dimitropoulos merged commit a03c098 into Kong:develop Jun 25, 2021
@NickHackman
Copy link
Contributor Author

@dimitropoulos whoops, missed the @flow annotation good catch!

@dimitropoulos
Copy link
Contributor

no worries @NickHackman! :)

Thanks for your contribution!

@yoosif0
Copy link

yoosif0 commented Aug 17, 2021

Is this in the current release?

@develohpanda
Copy link
Contributor

Hey @youssefsharief, this will be a part of the next release (which should be out this month)

@yoosif0
Copy link

yoosif0 commented Aug 17, 2021

@develohpanda thanks

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.

Add support for folder/request group descriptions
5 participants