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

SELF-168: Add InputNumber Component #446

Merged
merged 2 commits into from
Feb 14, 2024
Merged

Conversation

NateWaldschmidt
Copy link
Contributor

@NateWaldschmidt NateWaldschmidt commented Feb 14, 2024

JIRA

Description

  • The existing CurrencyInput had a bug with input, instead of fixing we are opting to utilize the PrimeVue InputNumber component instead

Screenshots

image

Reviewer Checklist

This section is to be filled out by reviewers

Testing

  • This code was tested by somebody other than the developer. Do not merge until this has been done.

Copy link

guardrails bot commented Feb 14, 2024

⚠️ We detected 7 security issues in this pull request:

Vulnerable Libraries (7)
Severity Details
High pkg:npm/[email protected] (t) upgrade to: > 3.1.6
Critical pkg:npm/[email protected] (t) upgrade to: > 10.3.1
Medium pkg:npm/[email protected] (t) upgrade to: > 8.19.0
Critical pkg:npm/[email protected] (t) upgrade to: > 8.2.5
Medium pkg:npm/[email protected] (t) upgrade to: 8.4.31
Medium pkg:npm/[email protected] (t) upgrade to: > 8.7.1
Critical pkg:npm/[email protected] (t) upgrade to: > 16.8.3

More info on how to fix Vulnerable Libraries in JavaScript.


👉 Go to the dashboard for detailed results.

📥 Happy? Share your feedback with us.

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-446.d11k469e311m4w.amplifyapp.com

@NateWaldschmidt NateWaldschmidt force-pushed the SELF-168/add-input-number branch 2 times, most recently from b0efbb2 to 755ebe1 Compare February 14, 2024 15:26
@NateWaldschmidt NateWaldschmidt marked this pull request as ready for review February 14, 2024 15:31
@NateWaldschmidt NateWaldschmidt requested a review from a team as a code owner February 14, 2024 15:31
@juanfriss
Copy link
Contributor

did we evaluate making this change on dashboard-vue itself? meaning, replacing all instances of CurrencyInput with InputNumber in the dashboard, and in this repo just removing CurrencyInput

@NateWaldschmidt
Copy link
Contributor Author

NateWaldschmidt commented Feb 14, 2024

did we evaluate making this change on dashboard-vue itself? meaning, replacing all instances of CurrencyInput with InputNumber in the dashboard, and in this repo just removing CurrencyInput

Yeah there are only 2 places where the CurrencyInput is currently being used within dashboard-vue should be a small PR to get these tested and updated

Thats a good point of removing the CurrencyInput now, Ill update this to remove it

@juanfriss
Copy link
Contributor

juanfriss commented Feb 14, 2024

so why do we have to "Add the InputNumber component" here?

@NateWaldschmidt NateWaldschmidt force-pushed the SELF-168/add-input-number branch from 755ebe1 to c183fce Compare February 14, 2024 16:10
@NateWaldschmidt
Copy link
Contributor Author

NateWaldschmidt commented Feb 14, 2024

so why do we have to "Add the InputNumber component" here?

I think understand what you're saying. This input is going to function as both a CurrencyInput and a number input thus the renaming of the component

Copy link
Contributor

@kencrim kencrim left a comment

Choose a reason for hiding this comment

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

This is a nitpick, but if we have TextInput, I'd personally prefer "NumberInput" over "InputNumber" for consistency. Approving pending the resolution of the above convo.

re: the above, my two cents is that we should probably have all of our inputs in the same place, so as long as we use ui-components for our other inputs, it's probably the place for this one, as well. I don't have strong feelings one way or the other, though!

@juanfriss
Copy link
Contributor

ok, I thought we were using PrimeVue InputNumber straight away, that's why I was suggesting just using that in dashboard-vue. If we are creating a new personalized component, then it makes sense to have it here

@NateWaldschmidt
Copy link
Contributor Author

This is a nitpick, but if we have TextInput, I'd personally prefer "NumberInput" over "InputNumber" for consistency. Approving pending the resolution of the above convo.

re: the above, my two cents is that we should probably have all of our inputs in the same place, so as long as we use ui-components for our other inputs, it's probably the place for this one, as well. I don't have strong feelings one way or the other, though!

@kencrim

Fair point on the naming convention. My thought is that TextInput is also going to need to be updated to PrimeVue in the future and I think the InputNumber is a better convention since it makes it easier to find the Input.... With that refactor we could create a InputText and then deprecate the TextInput component while we refactor over to InputText

Do you think that is a good idea or should we update this to be NumberInput? I don't feel strongly either way

@NateWaldschmidt NateWaldschmidt force-pushed the SELF-168/add-input-number branch from 06ed254 to 2b54019 Compare February 14, 2024 22:19
@NateWaldschmidt NateWaldschmidt merged commit 796e8da into main Feb 14, 2024
4 of 5 checks passed
@NateWaldschmidt NateWaldschmidt deleted the SELF-168/add-input-number branch February 14, 2024 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants