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(environment-variable): import feature #247

Merged
merged 23 commits into from Aug 4, 2022
Merged

Conversation

bdebon
Copy link
Contributor

@bdebon bdebon commented Jul 26, 2022

What does this PR do?

Link to the JIRA ticket

todo:

  • toggle all true false not working on false
  • input select small does not change its value accordingly to the Control. If the field value is set dynamically through setValue the select will not show that
  • make the window scrollable and wider (see with Remi because the modal is a component tricky that I have never put my hand in) (Created a JIRA ticket for that)
  • plugging the store and the actual api
  • dynamic submit button text saying how many override and new
  • design and refactoring of dropdown part
  • when select is valid would be cool to have the same outline has input
  • switching input value for textarea and try to make an okay design for that... (don't know how I'm gonna do yet) – I won't do that because it complexifies a lot everything and it is how it was implemented on v2 and I've never heard any complain about that
  • error message are breaking the design
  • validation rules for each fields
  • warning tooltip on import are glitchy, they pop too low and I don't know why (Created a JIRA ticket for that)
  • unhide all (needs to create an UI store for this purpose only)
  • saying update instead of overwritten because overwrite and override are different concepts...
  • changing the warning check to make the distinction between override (same key / scope below) and overwrite (same key / same scope)
  • Adding the toggle overwrite / or no overwrite
  • reworking tests

Screenshot of the actual work where I forced a wider a width
CleanShot 2022-07-26 at 18 47 12@2x

@evoxmusic
Copy link
Contributor

Preview environments were automatically created via Qovery.
Click on the links below to follow their deployments and use them.
👉 [PR] staging - Feat/env variable import - 2022-07-26T09:11:56Z
👉 [PR] [PR] staging - wip - 2022-07-21T12:29:27Z - Feat/env variable import - 2022-07-26T09:11:56Z

@nx-cloud
Copy link

nx-cloud bot commented Jul 26, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c7d7b3e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

@bdebon bdebon changed the title Feat/env variable import Feat(environment-variable): import feature Jul 26, 2022
@bdebon
Copy link
Contributor Author

bdebon commented Jul 26, 2022

For now, any error is breaking the layout..
CleanShot 2022-07-26 at 18 48 58@2x

@bdebon
Copy link
Contributor Author

bdebon commented Jul 28, 2022

I have a bug super strange.
When I select all the text of a text input and delete it, no changes is triggered. Then no validation is done by react-hook-form.
I checked many direction, I thought the problem was first coming from the way I reset the form, then I thought it was coming from react-hook-form, but it appears it's coming from the way we set the value of our input.
According to this stack overflow, we should rather use value than defaultValue.
https://stackoverflow.com/questions/64548202/react-input-change-not-firing-when-select-all-to-delete

But of course, using value breaks our forms...
Investigating on it.

@bdebon
Copy link
Contributor Author

bdebon commented Jul 28, 2022

I have a bug super strange. When I select all the text of a text input and delete it, no changes is triggered. Then no validation is done by react-hook-form. I checked many direction, I thought the problem was first coming from the way I reset the form, then I thought it was coming from react-hook-form, but it appears it's coming from the way we set the value of our input. According to this stack overflow, we should rather use value than defaultValue. https://stackoverflow.com/questions/64548202/react-input-change-not-firing-when-select-all-to-delete

But of course, using value breaks our forms... Investigating on it.

I fixed that by changing the onChange to onInput

Copy link
Member

@RemiBonnet RemiBonnet left a comment

Choose a reason for hiding this comment

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

Hey @bdebon !!
Well done very clean and the coverage is nice 💯

Here are my suggestions in addition to remarks in the code:

  • We don't have the export with terraform, I think you can remove it

Capture d’écran 2022-08-04 à 16 21 53

  • We have an white space at the right and left of the small input, it's little bit weird

Capture d’écran 2022-08-04 à 16 25 14

  • We must not use Slices outside the domains, we have React context for small UI behavior

  • Missing hover state on the drop zone

Capture d’écran 2022-08-04 à 16 31 09

  • We don't have the "Warning block" and the table title are not in the good place (you integration vs the design)

Capture d’écran 2022-08-04 à 16 37 24

Capture d’écran 2022-08-04 à 16 37 20

  • The warning icon looks too big, should be 20px of width and the margin-right is not the same with the design

@bdebon
Copy link
Contributor Author

bdebon commented Aug 4, 2022

I created the component for the warning block, we can use it, but I removed it because the logic has changed a lot.
Now we have the toggle that actually can change completely the text inside the warning block and we also can't compute frontend side the number of variable that will actually be overridden. To see with @alecarrano if we want to keep the warning box and in which context here.

@bdebon
Copy link
Contributor Author

bdebon commented Aug 4, 2022

Gg for the very precise review and suggestions! Gg bg

@bdebon bdebon merged commit 076e5f4 into staging Aug 4, 2022
@bdebon bdebon deleted the feat/env-variable-import branch August 4, 2022 17:12
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

3 participants