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

Allow users to configure what is shown on environment variable template tags #4277

Merged

Conversation

develohpanda
Copy link
Contributor

@develohpanda develohpanda commented Dec 7, 2021

changelog(Improvements): Now you can configure whether values are shown for environment variable template tags

Demo

https://www.loom.com/share/0fee8e05315741c5bfe3f8c8c4d19dce

Summary

This PR does:

  • Add a boolean in Preferences > General to uncover variables
  • Persist the setting in settings instead of in-memory
  • Remove all the prop drilling
  • Will swap what is shown in the tool-tip when the option is enabled vs disabled (see demo recording below)

This PR doesn't:

In the future:

  • We could give users more control over what they want to see in the template tag. The three bits of information we have are:
    • the source (root, or the environment name, or the folder name)
    • the variable name (foo, host, etc)
    • the value (bar, https://insomnia.rest)

The initial request for this came from #754, was resolved in #1274, but it got lost somewhere. Seems like a valuable thing to have. The only place this is configurable right now is via a hotkey, but I added a setting for it.

TODO:

  • QA
  • Setting name
  • Confirm setting help text

Closes INS-1226

@develohpanda develohpanda self-assigned this Dec 7, 2021
@develohpanda develohpanda removed the request for review from dimitropoulos December 7, 2021 06:30
@develohpanda
Copy link
Contributor Author

@wdawson / @falondarville here's a demo of this feature (which already existed, I just added a setting for it so users can access it).

https://www.loom.com/share/0fee8e05315741c5bfe3f8c8c4d19dce

Here's what the title and tooltip currently show

image

Could you please let me know if you'd like these reworded?

@develohpanda develohpanda marked this pull request as ready for review December 8, 2021 05:09
@develohpanda develohpanda changed the title Remove isVariableUncovered prop drilling Allow users to configure what is shown on environment variable template tags Dec 8, 2021
@wdawson
Copy link
Contributor

wdawson commented Dec 8, 2021

@develohpanda

Could you please let me know if you'd like these reworded?

Is "Show variable values" clearer for the title? The tooltip could read: "Shows the source and value of the variables instead of the variable name on the template tags"

CC - @falondarville

@falondarville
Copy link
Contributor

@develohpanda @wdawson thoughts?

If checked, reveals the environment variable source and value in the template tag. Otherwise, hover over the template tag to see the source and value.

@develohpanda
Copy link
Contributor Author

develohpanda commented Dec 8, 2021

I like that too, the fallback! It's informative. @falondarville Does that use Wils's title suggestion of "Show variable values"?

@falondarville
Copy link
Contributor

Should it be "Show variable source and values"? I lean towards wanting to be explicit and we have some longer check box titles, so it won't look off

@wdawson
Copy link
Contributor

wdawson commented Dec 8, 2021

Should it be "Show variable source and values"? I lean towards wanting to be explicit and we have some longer check box titles, so it won't look off

Agree! As long as there's the space to do it.

@develohpanda
Copy link
Contributor Author

image

Looks good now!

@vercel vercel bot temporarily deployed to Preview December 8, 2021 23:48 Inactive
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.

noticed we're missing typings in the nunjucks extension for isVariableUncovered. very tempted to add the handle render types as well, but I didn't.. heh. b87a87c did it if you wanna take a look (it's outside of the line scope of this changeset on GitHub so I can't comment on it here)

await this._updateIsVariableUncovered();
},
],
[hotKeyRefs.ENVIRONMENT_UNCOVER_VARIABLES, this._updateIsVariableUncovered],
Copy link
Contributor

Choose a reason for hiding this comment

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

so, looking at this, this won't totally work, yet, because we need to do some work to get the current restart requirement to go away, right? I'm on board with not fixing that in this PR -> just wanted to make sure I'm understanding it correctly. If that's the case, can you please make a ticket for fixing that part so we can triage for the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, using the hotkey exhibits the same behavior as only changing the option in preferences and not restarting the app. I've got #4276 as a spike (backed by INS-1245 in cycle 59) to address removing the restart requirement by fixing the underlying bug.

const valueAndContext = contextForKey ? `{${contextForKey}}: ${title}` : title;

// Swap what's shown in the tooltip vs the innerHTML
innerHTML = isVariableUncovered ? valueAndContext : cleanedStr;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish there was some way to not have to do all the drilling inside of this extension code. There's no "react" here so a hook selector makes no sense... so it is what it is I guess. curious, though, if you've thought of anything (even, that we could potentially try in the future) to get this extension code to be less of an ugly duckling in terms of its capabilities compared to the rest of our codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm planning to review the extension code in #4276, and will make this nicer to use. INS-1245

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.

it's really incredible. TWO HUNDRED and THIRTY NINE usages in the app before, when all we ever needed was:

  • 1 usage for the settings tab in preferences (which, haha, we didn't bother having before, thanks for adding!!)
  • 1 usage for the hotkey execution logic
  • 2 usages in the nunjucks-tags extension

and.... that's it!

wonderful! wonderful wonderful!


I pushed some commits to https://github.com/dimitropoulos/insomnia/commits/feature/ins-1226-remove-isvariableuncovered-prop-drilling referenced in the feedback.

<BooleanSetting
label="Show variable source and values"
help="If checked, reveals the environment variable source and value in the template tag. Otherwise, hover over the template tag to see the source and value."
setting="isVariableUncovered"
Copy link
Contributor

Choose a reason for hiding this comment

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

nice to have a variable name that matches the new label somewhat, but the migration may not be worth the benefit now its usage is simpler to understand. Besides that, nice job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great point 👍🏽 renamed to showVariableSourceAndValue. The good thing is because this didn't previously exist in the database, no migration was necessary, just a find and replace in code. 2e5d639

Copy link
Contributor

Choose a reason for hiding this comment

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

GREAT point @jackkav. speaking personally, I think my head was too deep in the current falling sands of low-level concepts to notice this high-level thing.

@vercel vercel bot temporarily deployed to Preview December 13, 2021 07:07 Inactive
@develohpanda
Copy link
Contributor Author

@dimitropoulos thank you for the commits! Have pulled them in 💯

@develohpanda develohpanda enabled auto-merge (squash) December 13, 2021 07:19
@develohpanda develohpanda merged commit 9586ee7 into develop Dec 13, 2021
@develohpanda develohpanda deleted the feature/ins-1226-remove-isvariableuncovered-prop-drilling branch December 13, 2021 07:45
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.

None yet

5 participants