Skip to content

Allow switching sub-environment inside Manage Environment modal #2891

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

Merged
merged 4 commits into from
May 19, 2021

Conversation

cobwebsonsale
Copy link
Contributor

@cobwebsonsale cobwebsonsale commented Dec 2, 2020

Closes #2761

  • Added a button next to the sub-environment name to activate it
  • Added a filled button next to the current active sub-environment
  • Added helpful text in tooltips
  • Renamed occurrences of activeEnvironment to selectedEnvironment in WorkspaceEnvironmentsEditModal (to avoid confusion) -- separated in another commit, for easier review/revert.

Caveat:
Could not re-render the currently selected environment in the right-pane after activation, so showing the activated environment instead. The re-render only makes sense when Base Environment is selected, as a different active environment changes the validations on variables. Tried:

  • calling _load,
  • adding activeEnvironmentId to state inside WorkspaceEnvironmentsEditModal and setting state, and
  • deriving activeEnvironmentId from activeWorkspaceMeta in Wrapper, as that is what gets updated in handleChangeEnvironment

Demo:
switch-environment

@develohpanda
Copy link
Contributor

Wow, thanks a lot for tackling this @cobwebsonsale! 🤗

@develohpanda develohpanda self-requested a review December 2, 2020 23:33
@CLAassistant
Copy link

CLAassistant commented Dec 6, 2020

CLA assistant check
All committers have signed the CLA.

@cobwebsonsale
Copy link
Contributor Author

Requesting a review here.

@develohpanda
Copy link
Contributor

I'm sorry about the mega delays @cobwebsonsale 😞 I'll try to at least give a prelim review this this week

@vercel vercel bot temporarily deployed to Preview May 11, 2021 04:54 Inactive
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.

Thank you for the PR, QA'd locally and seems to be working as intended, and thanks for splitting up your work into individual commits which made the rename and feature easier to review. 🙌🏽

My apologies for the delays. Unfortunately the team is spread quite thin at the moment, evident by the sheer number of open-source PRs we need to get through. We currently have a code-freeze in place for migrating to typescript, but once that is done I'll come back to this PR to merge it.

@cobwebsonsale
Copy link
Contributor Author

cobwebsonsale commented May 11, 2021

seems to be working as intended

Nice, that's good to know.

And no worries about the delay. I want to contribute to help, not to add stress. xD So please take your time and merge this accordingly. Bumped only so it stays on the radar.

Let me know when you come around to this and there are conflicts, I'm surprised there weren't already.

@develohpanda
Copy link
Contributor

Sounds good 👍🏽

This is a note to myself to double-check the caveat you describe above and whether that's a concern right now; with the testing done so far it seemed to be fine

@develohpanda develohpanda self-requested a review May 13, 2021 06:06
@dimitropoulos
Copy link
Contributor

I updated this branch for TypeScript: it seems to be working well -

https://github.com/dimitropoulos/insomnia/commits/feature/switch-env-ts

The first commit is yours (please let me know if I messed up anything on the revival) and the second commit is to fix some TypeScript errors now that we're actually using types for react-sortable-hoc.

Please take a look. If everything is ok, I can push that branch to this one and update this PR, at which point we should be ready (or nearly ready) to go.

@develohpanda
Copy link
Contributor

develohpanda commented May 17, 2021

@dimitropoulos having 2286466 (#2891) and d1c4ae2 (#2891) separate certainly was helpful in processing this PR, but if that's tricky to retain it's okay. I briefly checked your branch and dimitropoulos@c5eff8a looks correct.

Please push those changes up to cobwebsonsale:feature/switch-env and we'll run through another code-review and QA round.

cobwebsonsale and others added 3 commits May 17, 2021 12:10
- Show env status next to name
- Add tooltip for statuses
- Show env after activation
note the end of line 5.  that was the reason none of these types were used before.
@dimitropoulos
Copy link
Contributor

@develohpanda sure, I un-squashed the rename and pushed to the PR's branch.

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.

Alright, double checked again and it looks good to me

Could not re-render the currently selected environment in the right-pane after activation, so showing the activated environment instead. The re-render only makes sense when Base Environment is selected, as a different active environment changes the validations on variables.

I worked through this caveat you described; it's interesting that the re-render doesn't occur but when hovering over a template tag in the base environment the sub environment in that tooltip does change but the resolved value does not. This leads me to believe there must be a bug somewhere and that it should work automatically.

Regardless, I still think what you have is fine for this iteration. 🎉

@@ -225,7 +261,7 @@ class WorkspaceEnvironmentsEditModal extends PureComponent<Props, State> {

// Unset active environment if it's being deleted
if (activeEnvironmentId === environment._id) {
await handleChangeEnvironment(null);
handleChangeEnvironment(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Removal of this await actually shows a bug where Wrapper._handleChangeEnvironment function swallows a promise. It should be awaited correctly so we don't proceed to delete an environment until the change is complete.

Can be handled separate to this PR

@develohpanda develohpanda requested review from DMarby and reynolek and removed request for DMarby May 19, 2021 04:55
Copy link
Contributor

@reynolek reynolek left a comment

Choose a reason for hiding this comment

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

Product perspective: Good addition, once merged we should probably do a design pass but that doesnt have to happen right now.

@develohpanda develohpanda merged commit 212ca5d into Kong:develop May 19, 2021
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 ability to see what environment is currently selected, and a way to change environment from Manage Environment modal
5 participants