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

Improve visibilty of the drag pane when viewed vertically #2713

Merged
merged 5 commits into from Sep 9, 2021

Conversation

rmsy
Copy link
Contributor

@rmsy rmsy commented Oct 12, 2020

This PR fixes #2584, by making forceVerticalLayout also apply to the "Design" and "Test" tabs. Here's an overview of the changes that were made:

  • Rename the settings options (for consistency) as described below. (I realize the requirement listed in Design and Test tabs should respect vertical layout preferences #2584 was to change the text to read "Force vertical layout", but "force" feels a little weird to me, as both of the settings below are supported features, not something that needs to be "forced". If that is still the desired verbiage, let me know and I'll change the PR appropriately).
  • Update CSS to apply appropriate layout for design/test in vertical mode (this was the primary issue)

One additional note: there is some custom logic in app.js to support the draggable resize bar in the Debug tab, both in the horizontal and the vertical layouts. The Design/Test tabs do not have this resize bar implemented in the horizontal layout, so I didn't bring it over for the vertical layout, either. (I couldn't think of a simple and clean way to implement it, and I didn't want to clutter app.js any further). Maybe implementing the resize bar for Design/Test could be a future improvement.

Screenshots:

image

image

image

Closes INS-923

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.

One additional note: there is some custom logic in app.js to support the draggable resize bar in the Debug tab, both in the horizontal and the vertical layouts. The Design/Test tabs do not have this resize bar implemented in the horizontal layout, so I didn't bring it over for the vertical layout, either. (I couldn't think of a simple and clean way to implement it, and I didn't want to clutter app.js any further). Maybe implementing the resize bar for Design/Test could be a future improvement.

Nice observation, #2583 is this very issue, and someone is already working through it 👏

This PR looks good to me; I agree with the verbiage changes. They are subtle and convey the same message.

Copy link
Contributor

@nijikokun nijikokun left a comment

Choose a reason for hiding this comment

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

Rename the settings options (for consistency) as described below. (I realize the requirement listed in #2584 was to change the text to read "Force vertical layout", but "force" feels a little weird to me, as both of the settings below are supported features, not something that needs to be "forced". If that is still the desired verbiage, let me know and I'll change the PR appropriately).
"Force bulk header editor" -> "Use bulk header editor"
"Vertical request/response layout" -> "Use vertical layout"

Let's change the "bulk header editor" text in a separate PR as it is un-related to this feature, it's a side-effect of the discussion occurred here and should reference this PR as such.

I'm okay with Use vertical layout, this does make me think we should add information tooltips what this setting is going to change or implies for the user as without immediate visual feedback it could be un-clear compared to previous text.

@rmsy rmsy force-pushed the feature/fix-vertical-layout-issue branch from 19ad2ce to 496afac Compare October 13, 2020 02:02
@rmsy
Copy link
Contributor Author

rmsy commented Oct 13, 2020

Let's change the "bulk header editor" text in a separate PR as it is un-related to this feature, it's a side-effect of the discussion occurred here and should reference this PR as such.

Thank you for the feedback, @nijikokun, that makes sense -- I will move the bulk header editor change to another PR.

this does make me think we should add information tooltips what this setting is going to change or implies for the user as without immediate visual feedback it could be un-clear compared to previous text.

I am struggling to come up with a descriptive label for the help text. This is the best I could muster, but perhaps someone else here will have a better suggestion:

"Stack application panels vertically instead of horizontally"

image

I've updated this PR pending other suggestions.

@rmsy rmsy requested a review from nijikokun October 13, 2020 02:04
rmsy added a commit to rmsy/insomnia that referenced this pull request Oct 13, 2020
This change is to be consistent with Kong#2713.
develohpanda pushed a commit that referenced this pull request Oct 13, 2020
This change is to be consistent with #2713.
@nijikokun
Copy link
Contributor

nijikokun commented Oct 20, 2020

I am struggling to come up with a descriptive label for the help text. This is the best I could muster, but perhaps someone else here will have a better suggestion:

It's definitely a good start! My only suggestion is providing an example after panels, taking inspiration from the previous we could do something like:

Stack application panels (e.g. request / response) vertically instead of horizontally.

Thank you for the feedback, @nijikokun, that makes sense -- I will move the bulk header editor change to another PR.

🤗 Thank you for the great PR 🎉

@develohpanda
Copy link
Contributor

FYI @rmsy I think there might actually be some conflicts with #2712, as that PR improves the overall layout logic of the app

@develohpanda
Copy link
Contributor

FYI @rmsy I think there might actually be some conflicts with #2712, as that PR improves the overall layout logic of the app

Following up on this - it looks like with the changes introduced in #2712, forcing vertical layout seems to be fixed as a side-effect. 🤯 I really appreciate the time that you put into this fix, but with the refactoring and removal of duplication in #2712, my preference would be to go ahead with the overall fix in that PR.

I did, however, notice some border styling that you introduced in this PR, which might fix another related bug I found during recent QA - #2787. That, coupled with the re-naming changes you have suggested in preferences are still valid.

@rmsy
Copy link
Contributor Author

rmsy commented Oct 27, 2020

Ok, thank you @develohpanda! I will rebase this PR on top of #2712 later today and reduce the scope to include only the border fix and renames (including wording adjustment as suggested by @nijikokun). I appreciate all of the feedback 😊

@develohpanda
Copy link
Contributor

Hi @rmsy, apologies for the big delays but #2712 has now been merged to develop so this should be unblocked 🙂

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@develohpanda develohpanda changed the title Make forceVerticalLayout also apply to design/test Improve visibilty of the drag pane when viewed vertically Sep 9, 2021
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.

I updated the scope of this PR because there was some duplication with another one and half of the feature already exists in develop. 😄

My apologies for the long delay, but thank you very much for the PR and improvement. We really appreciate it 😊

QA

Before After
image image
image image
image image

@develohpanda develohpanda enabled auto-merge (squash) September 9, 2021 07:12
@develohpanda develohpanda merged commit cb17482 into Kong:develop Sep 9, 2021
@rmsy rmsy deleted the feature/fix-vertical-layout-issue branch September 11, 2021 01:40
@rmsy
Copy link
Contributor Author

rmsy commented Sep 11, 2021

@develohpanda Sorry I never got around to cleaning up my branch, thanks for doing that and merging, and thanks for maintaining such a great developer tool ❤️

@develohpanda
Copy link
Contributor

No worries at all 😊

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.

Design and Test tabs should respect vertical layout preferences
5 participants