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

enhancement: Adding UI for variables of any json type #2414

Merged
merged 17 commits into from
May 22, 2024

Conversation

WillRaphaelson
Copy link
Contributor

@WillRaphaelson WillRaphaelson commented May 15, 2024

This PR:

  • Updates create and edit modals to use the json input, which formats highlights and validates.
  • Links the name to the opening of the edit modal
  • Retains the value column for any variable with length <= 64 characters, and provides a link that opens the modal if its longer.
  • Correct a bug that was causing the edit modal to display backticks.

For the team - should i be reaching back into the types more than i am?

image image

All thoughts welcome, thanks.

@WillRaphaelson WillRaphaelson marked this pull request as ready for review May 16, 2024 15:20
@WillRaphaelson WillRaphaelson requested a review from a team as a code owner May 16, 2024 15:20
Copy link
Collaborator

@collincchoy collincchoy left a comment

Choose a reason for hiding this comment

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

This looks good to me! Mostly non-blocking comments - only 1 comment I think might need to be addressed around handling a variable edit/update from a clicked VariableLink. I think(?) I'm right there that atm you'd update and not see your updated data.

should i be reaching back into the types more than i am?

Hm 🤔 not sure what you mean.

src/components/VariableLink.vue Outdated Show resolved Hide resolved
src/components/VariableLink.vue Outdated Show resolved Hide resolved
src/components/VariablesTable.vue Outdated Show resolved Hide resolved
src/components/VariableCreateModal.vue Outdated Show resolved Hide resolved
src/components/VariablesTable.vue Outdated Show resolved Hide resolved
src/components/VariablesTable.vue Outdated Show resolved Hide resolved
src/components/VariablesTable.vue Outdated Show resolved Hide resolved
@WillRaphaelson
Copy link
Contributor Author

This looks good to me! Mostly non-blocking comments - only 1 comment I think might need to be addressed around handling a variable edit/update from a clicked VariableLink. I think(?) I'm right there that atm you'd update and not see your updated data.

should i be reaching back into the types more than i am?

Hm 🤔 not sure what you mean.

I mean like should i be changing the types for variables from stringg to like string|int etc?

Copy link
Collaborator

@collincchoy collincchoy left a comment

Choose a reason for hiding this comment

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

cha-ching

@WillRaphaelson WillRaphaelson merged commit 52a1e1e into main May 22, 2024
3 checks passed
@WillRaphaelson WillRaphaelson deleted the variables-json branch May 22, 2024 15:03
@WillRaphaelson WillRaphaelson restored the variables-json branch May 23, 2024 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants