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: Key-value pair view for environment[INS-4256] #8071

Merged
merged 17 commits into from
Oct 28, 2024

Conversation

cwangsmv
Copy link
Contributor

@cwangsmv cwangsmv commented Oct 12, 2024

Highlight and Changes:

Add new KV-pair view for environment

  • Collection environment
Screenshot 2024-10-17 at 15 52 52
  • Global environment
Screenshot 2024-10-17 at 15 51 59
  • Folder environment
Screenshot 2024-10-17 at 11 26 25
  • UI Change:
    • New key-pair table for collection environment, global environment and folder environment UI
    • Prompt JSON editor modal
    • Add button to switch JSON view & kv pair view
    • Confirm naming and default view for environment
  • Models Change:
    • Add type and kv pair data attribute to environment and request-group data model
    • Ensure that newly add attributes won't add new attributes to old datas in git sync
  • Data transform
    • JSON and kv pair data transform logic

@cwangsmv cwangsmv marked this pull request as draft October 12, 2024 05:31
@cwangsmv cwangsmv force-pushed the feat/env-config-table-view branch 2 times, most recently from ee16325 to 7578f6c Compare October 17, 2024 07:42
@cwangsmv cwangsmv marked this pull request as ready for review October 17, 2024 07:56
@cwangsmv cwangsmv changed the title feat: Key-value pair view for environment feat: Key-value pair view for environment[INS-4256] Oct 17, 2024
Copy link
Contributor

@ihexxa ihexxa left a comment

Choose a reason for hiding this comment

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

Had review as pass 1 and will do more checks later.

kvPair.push({
id: generateId('envPair'),
name: key,
value: isValObject ? JSON.stringify(val) : String(val),
Copy link
Contributor

Choose a reason for hiding this comment

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

JSON.stringify may not work with nunjucks tags as values.

Copy link
Contributor Author

@cwangsmv cwangsmv Oct 22, 2024

Choose a reason for hiding this comment

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

Show a modal to block user from changing to table view when there's JSON error
Screenshot 2024-10-22 at 17 19 14

id: string;
name: string;
value: string;
type: EnvironmentKvPairDataType;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we infer the data types for other primitives like number, boolean etc?
Currently if I add a number or boolean it is recognized as string in the table editor.
This might be difficult if we try to infer nunjucks templates that are not inside quotes but not quite sure about it.

},
];

const renderPairItem = (kvPair: EnvironmentKvPairData) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should turn this into a component in the module scope?

}, []);

return (
<div className="p-[--padding-sm] min-w-max h-full overflow-hidden flex flex-col">
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there is extra padding around the toolbar. If we want we could add padding in the key-value editor ListBox bellow

Suggested change
<div className="p-[--padding-sm] min-w-max h-full overflow-hidden flex flex-col">
<div className="min-w-max h-full overflow-hidden flex flex-col">

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will remove the padding for toolbar

@cwangsmv cwangsmv enabled auto-merge (squash) October 28, 2024 02:15
@cwangsmv cwangsmv merged commit 0bae709 into develop Oct 28, 2024
8 checks passed
@cwangsmv cwangsmv deleted the feat/env-config-table-view branch October 28, 2024 02:26
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.

3 participants