Api Key settings frontend #2403

CDimonaco · 2024-03-11T11:47:20Z

Description

This PR adds the Api Key settings frontend.
It includes the generation of new api key with custom expiration

New settings page

API Key settings modal

How was this tested?

Automated tests

arbulu89

Hey @CDimonaco ,
Great job.
I have noted many comments, excluding by now the Input/Switch components.
Let me know what do you think

assets/js/common/ApiKeySettingsModal/ApiKeySettingsModal.jsx

arbulu89 · 2024-03-12T10:24:26Z

assets/js/common/ApiKeySettingsModal/ApiKeySettingsModal.jsx

+      open={open}
+      onClose={onClose}
+    >
+      <div className="flex flex-col my-5">


Checking the modal in storybook, I would say that some margins don't follow the mock up.
Can you confirm this with @jagabomb ?
I'm fine, but I want Jurgen's approval there hehe

Hi @CDimonaco, here is my feedback regarding the spacing, and number input for the default state:

Reduce and standardise the spacing between the heading and between line of content (highlighted in red block with pixel height dimensions). Ideally they were 16px in the design.

Remove the margin below the close button. There is already a spacing which is provided by the padding of the modal.

Resolve this UI addition and subtraction indicator for the numeric field since there is a native component (Chrome) which creates a duplication of the same function.

For the point 1 and 2, I will standardize the space, for the last point, this is a custom component from the same component library we use for other inputs, if we fallback to the native component, we should design all the style and create a custom component to match all the others input design.

Why this component is bad? We could of course fallback to the native component but we need to create a custom component which I think it's a big effort and out the scope of this pr.

I tested on multiple browser and I cannot reproduce the "dual" indicators on the number component, but again, if we need to write custom logic on top of the custom or native component it's not an easy task and out of the scope of this pr.

cc @dottorblaster maybe my difficulty estimation is wrong, wdyt?

@CDimonaco I agree, if the effort is too large for the numeric input to be improved then we can re-look at this in another PR. I just wanted to note the input behaviour since Alberto also raised it too.

Point 1 and 2 are perfectly addressable in this PR, when it comes to point 3 I didn't quite get your feedback @jagabomb, my advice is to merge and proceed incrementally to do this kind of work on the design system, which we couldn't foresee.

The alternative is to adopt a text input for the moment, but I would suggest otherwise since with a number input we can ensure nothing strange is submitted.

Space standardized, you can check in storybook but this is the screenshot, there are also no more margin under the button, only the modal margins.

@dottorblaster It is easier to see the issue with the duplicated numeric addition and subtraction (arrow up and arrow down) buttons below. But as I said before, I am happy to defer this to another PR and revisit it when we have time. 😊

assets/js/common/ApiKeySettingsModal/ApiKeySettingsModal.jsx

assets/js/pages/SettingsPage/SettingsPage.jsx

assets/js/pages/SettingsPage/SettingsPage.test.jsx

assets/js/common/ApiKeySettingsModal/ApiKeySettingsModal.jsx

arbulu89 · 2024-03-12T10:42:34Z

assets/js/common/ApiKeySettingsModal/ApiKeySettingsModal.jsx

+          </span>
+        )}
+        {generatedApiKey && (
+          <div className="flex flex-col my-1 mb-4">


Maybe this div could be subcomponent itself, as we use it (or should) the same content here in the modal and in the settings page.
What do you think?

Anyway, if Jurgen agrees that they should be different, so be it

If it helps us have consistency in the layout and implementation then I am happy for it to be a subcomponent.

If it's a subcomponent, we will have the same copy button?

CDimonaco · 2024-03-12T13:07:42Z

PR ENV
https://2403.prenv.trento.suse.com

abravosuse

Beautiful, Carmine!

Just two questions/remarks:

In the number field in the API Key Settings modal, when you write a number you have two sets of icons to increase/decrease the number. Shouldn't there be only one set, the one of the field itself?

Every time you enter the API Key Settings modal, the previous settings are cleared out. This is more a question for @jagabomb . Wouldn't it be better that the last set of settings is displayed instead? For instance, if the user turned on the the non expiration toggle, then, when you open the API Key Settings modal, it is still turned on. Or if the user set 1 year as expiration date then, when you open the modal, it is set to 1 year. Just wondering. The way it works now is also fine.

CDimonaco · 2024-03-12T13:55:14Z

Beautiful, Carmine!

Just two questions/remarks:

1. In the number field in the API Key Settings modal, when you write a number you have two sets of icons to increase/decrease the number. Shouldn't there be only one set, the one of the field itself?

2. Every time you enter the API Key Settings modal, the previous settings are cleared out. This is more a question for @jagabomb . Wouldn't it be better that the last set of settings is displayed instead? For instance, if the user turned on the the non expiration toggle, then, when you open the API Key Settings modal, it is still turned on. Or if the user set 1 year as expiration date then, when you open the modal, it is set to 1 year. Just wondering. The way it works now is also fine.

For the first question is a browser compatibility issues, I tested on some different browser and I did not find the issue, the library we are using is one of the best but these glitch could appear

For the second question, is technically feasible to do, but it's not easy because we should have compatibility with the previous key it's almost impossible to come back to the form with the expiration set, for example we know the date of the expiration but we don't know if you did 1 month or 30 days for example, I think it's better to leave the modal as is, resettinng it's state when the user close the modal

This keep consistency and usability imho

abravosuse · 2024-03-12T14:10:57Z

For the first question is a browser compatibility issues, I tested on some different browser and I did not find the issue, the library we are using is one of the best but these glitch could appear

OK. But, for the record, I'm not using anything out of ordinary (Chrome Version 122.0.6261.112).

For the second question, is technically feasible to do, but it's not easy because we should have compatibility with the previous key it's almost impossible to come back to the form with the expiration set, for example we know the date of the expiration but we don't know if you did 1 month or 30 days for example, I think it's better to leave the modal as is, resettinng it's state when the user close the modal

This keep consistency and usability imho

OK. I thought the data in the data entered in the modal was persistent. If it adds complexity to the implementation, then forget about it.

CDimonaco · 2024-03-12T14:14:19Z

For the first question is a browser compatibility issues, I tested on some different browser and I did not find the issue, the library we are using is one of the best but these glitch could appear

OK. But, for the record, I'm not using anything out of ordinary (Chrome Version 122.0.6261.112).

For the second question, is technically feasible to do, but it's not easy because we should have compatibility with the previous key it's almost impossible to come back to the form with the expiration set, for example we know the date of the expiration but we don't know if you did 1 month or 30 days for example, I think it's better to leave the modal as is, resettinng it's state when the user close the modal
This keep consistency and usability imho

OK. I thought the data in the data entered in the modal was persistent. If it adds complexity to the implementation, then forget about it.

The modal data is "persistent" across page reloads, but in the end the modal will generate an expiration timestamp.

We could remove the modal "reset" so the user can see the previous settings if they open and close the modal in the same page, but I think it's not really useful and can lead to some confusion about the usage

jagabomb · 2024-03-13T07:33:25Z

2. Every time you enter the API Key Settings modal, the previous settings are cleared out. This is more a question for @jagabomb . Wouldn't it be better that the last set of settings is displayed instead? For instance, if the user turned on the the non expiration toggle, then, when you open the API Key Settings modal, it is still turned on. Or if the user set 1 year as expiration date then, when you open the modal, it is set to 1 year. Just wondering. The way it works now is also fine.

I agree with you here @abravosuse, although I think this Storybook version doesn't really need to showcase the logic like that. I would say that behaviour should be in the AC so that we implement the logic in the product. Unless the team feels there is value to implement logic like that in the Storybook too?

arbulu89

Excellent work @CDimonaco
Codewise, ready to merge from my side (with some small nitpicks XD)

assets/css/input-number.css

assets/js/common/ApiKeySettingsModal/ApiKeySettingsModal.jsx

assets/js/common/ApiKeySettingsModal/ApiKeySettingsModal.stories.jsx

assets/js/common/ApiKeySettingsModal/ApiKeySettingsModal.test.jsx

assets/js/pages/SettingsPage/SettingsPage.jsx

jagabomb · 2024-03-13T07:59:55Z

I wonder if we should make this sentence case instead of title case, "Key will expires 12 Mar 2024"? What are you thoughts @abravosuse?
Can we have a hover state for the "Copy to Clipboard" button and after you click it show a tooltip that says "Copied to clipboard". I have included button states for reference.

assets/js/common/ApiKeySettingsModal/ApiKeySettingsModal.jsx

assets/js/common/Input/InputNumber.jsx

EMaksy

Hey man great work, what a big beautiful pr.
I wrote some comments and questions, but the code looks great.
For me LGTM!

CDimonaco · 2024-03-13T10:47:13Z

Merging as is

Creating 2 tech debts, one for the copy button, the other one for the numeric input.

CDimonaco added enhancement New feature or request javascript Pull requests that update Javascript code labels Mar 11, 2024

CDimonaco self-assigned this Mar 11, 2024

CDimonaco changed the title ~~Api Key setting frontend~~ Api Key settings frontend Mar 11, 2024

CDimonaco marked this pull request as ready for review March 11, 2024 17:32

CDimonaco added the env Create an ephimeral environment for the pr branch label Mar 11, 2024

CDimonaco force-pushed the api_settings_modal branch 2 times, most recently from 3f7d17b to 51e2c27 Compare March 11, 2024 20:52

CDimonaco requested review from rtorrero, arbulu89 and EMaksy March 12, 2024 10:30

arbulu89 reviewed Mar 12, 2024

View reviewed changes

CDimonaco and others added 11 commits March 12, 2024 11:52

Wip ApiKeySettings modal

44789a8

Allow classname injection on Modal component

ad8ffda

Remove overflow-hidden from the modal

c7f3d74

Add modal width on api key settings modal

f2496f9

ApiKeySettings modal with story

37a849f

ApiKeySettings modal tests

4aa754a

ApiKey expiration settings in settings page

f51635f

Custom Props injecting into input component

d7040d9

Expiration label when api key is infinite in settings page

9d96221

Disable min validation on input number in apikeysettings

b019bba

Add RcInputNumber to ApiKeySettings modal

718fc0b

CDimonaco force-pushed the api_settings_modal branch from 8297237 to 718fc0b Compare March 12, 2024 10:53

Addressing review feedbacks

1f0e400

CDimonaco force-pushed the api_settings_modal branch from d1d558e to 1f0e400 Compare March 12, 2024 11:22

New generated key in api key settings modal after generation

e0b6f94

CDimonaco requested a review from abravosuse March 12, 2024 13:06

abravosuse reviewed Mar 12, 2024

View reviewed changes

Add tests for api key displaying in settings page test

05d80bf

CDimonaco requested a review from jagabomb March 12, 2024 14:36

Add api key settings e2e tests

f4658d9

arbulu89 approved these changes Mar 13, 2024

View reviewed changes

EMaksy reviewed Mar 13, 2024

View reviewed changes

assets/js/common/ApiKeySettingsModal/ApiKeySettingsModal.jsx Show resolved Hide resolved

Addressing review feedbacks

1e3f251

CDimonaco force-pushed the api_settings_modal branch from 001d36f to 1e3f251 Compare March 13, 2024 09:44

EMaksy reviewed Mar 13, 2024

View reviewed changes

assets/js/common/Input/InputNumber.jsx Show resolved Hide resolved

EMaksy approved these changes Mar 13, 2024

View reviewed changes

CDimonaco merged commit f1beb76 into main Mar 13, 2024
24 checks passed

CDimonaco deleted the api_settings_modal branch March 13, 2024 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Api Key settings frontend #2403

Api Key settings frontend #2403

CDimonaco commented Mar 11, 2024 •

edited

Loading

arbulu89 left a comment

arbulu89 Mar 12, 2024

jagabomb Mar 13, 2024

CDimonaco Mar 13, 2024

jagabomb Mar 13, 2024

dottorblaster Mar 13, 2024

CDimonaco Mar 13, 2024

jagabomb Mar 13, 2024

arbulu89 Mar 12, 2024

arbulu89 Mar 12, 2024

jagabomb Mar 13, 2024

CDimonaco Mar 13, 2024

CDimonaco commented Mar 12, 2024

abravosuse left a comment •

edited

Loading

CDimonaco commented Mar 12, 2024

abravosuse commented Mar 12, 2024

CDimonaco commented Mar 12, 2024

jagabomb commented Mar 13, 2024

arbulu89 left a comment

jagabomb commented Mar 13, 2024

EMaksy left a comment

CDimonaco commented Mar 13, 2024

Api Key settings frontend #2403

Api Key settings frontend #2403

Conversation

CDimonaco commented Mar 11, 2024 • edited Loading

Description

How was this tested?

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CDimonaco commented Mar 12, 2024

abravosuse left a comment • edited Loading

Choose a reason for hiding this comment

CDimonaco commented Mar 12, 2024

abravosuse commented Mar 12, 2024

CDimonaco commented Mar 12, 2024

jagabomb commented Mar 13, 2024

arbulu89 left a comment

Choose a reason for hiding this comment

jagabomb commented Mar 13, 2024

EMaksy left a comment

Choose a reason for hiding this comment

CDimonaco commented Mar 13, 2024

CDimonaco commented Mar 11, 2024 •

edited

Loading

abravosuse left a comment •

edited

Loading