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

Add posibility to add custom helm repository #1368

Merged
merged 1 commit into from Dec 8, 2020

Conversation

pashevskii
Copy link
Contributor

@pashevskii pashevskii commented Nov 13, 2020

The commit adds the possibility to add a custom helm repository instead of selecting prepared ones
The commit adds new button in preferences - helm section
image

Warning! Implementation is ugly, need your comments on how to improve it.

@pashevskii pashevskii changed the title Add posibility to add custom repository Add posibility to add custom helm repository Nov 13, 2020
@pashevskii pashevskii force-pushed the add-custom-repo branch 3 times, most recently from 15061c5 to ba44a56 Compare November 13, 2020 13:50
@jakolehm jakolehm added area/ui enhancement New feature or request labels Nov 13, 2020
@pashevskii pashevskii force-pushed the add-custom-repo branch 7 times, most recently from ee86afb to f147877 Compare November 16, 2020 14:02
@aleksfront aleksfront self-requested a review November 25, 2020 12:23
Copy link
Contributor

@aleksfront aleksfront left a comment

Choose a reason for hiding this comment

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

Implementation is good, just need to add couple of UI fixes.

  1. Please put Add Custom Helm Repo dialog to the right of repo selector. You can do this by moving select and button into parent div element like this:
<h2><Trans>Helm</Trans></h2>
<div className="flex gaps">
  <Select id="HelmRepoSelect"
    placeholder={<Trans>Repositories</Trans>}
    isLoading={this.helmLoading}
    isDisabled={this.helmLoading}
    options={this.helmOptions}
    onChange={this.onRepoSelect}
    formatOptionLabel={this.formatHelmOptionLabel}
    controlShouldRenderValue={false}
    className="box grow"
  />
  <Button
    primary
    label={<Trans>Add Custom Helm Repo</Trans>}
    onClick={() => {AddHelmRepoDialog.open()}}
  />
</div>

helm repos

  1. Fix focus/tab states in newly created dialog. When you open it, focus appears not on the first input field, but on the second one. Same happens when you click "More" button - focus activated on last field in list.

  2. We can remove first title in dialog so the whole view would be like on the following screenshot
    add custom helm repo

Other visual fixes will be done in separate PRs since whole dialog view should be redefined.

@aleksfront
Copy link
Contributor

@pashevskii Also, please merge current master changes into your branch and resolve conflict in preferences.tsx and keep in mind that our codebase now require stricter lint rules, so you need to set semicolons everywhere at least.

Check out these commands yarn lint and yarn lint:fix.

@aleksfront aleksfront added this to the 4.0.1 milestone Dec 3, 2020
@pashevskii
Copy link
Contributor Author

@pashevskii Also, please merge current master changes into your branch and resolve conflict in preferences.tsx and keep in mind that our codebase now require stricter lint rules, so you need to set semicolons everywhere at least.

Check out these commands yarn lint and yarn lint:fix.

Ok, thank you for your comments, I'll try to fix it today.
Need your opinion about "More + " improvised accordion element, it looks not convenient enough, because you have to click on a small clickable area. Probably I should cover this one with some invisible area which has the same onClick function or replace the element with a button, what do you think about it?

@aleksfront
Copy link
Contributor

aleksfront commented Dec 3, 2020

@pashevskii Also, please merge current master changes into your branch and resolve conflict in preferences.tsx and keep in mind that our codebase now require stricter lint rules, so you need to set semicolons everywhere at least.
Check out these commands yarn lint and yarn lint:fix.

Ok, thank you for your comments, I'll try to fix it today.
Need your opinion about "More + " improvised accordion element, it looks not convenient enough, because you have to click on a small clickable area. Probably I should cover this one with some invisible area which has the same onClick function or replace the element with a button, what do you think about it?

Yes, you can wrap text and plus icon within <Button /> component, but not to forget to set plain prop on it.

@pashevskii pashevskii force-pushed the add-custom-repo branch 2 times, most recently from 8ff587a to 53307dc Compare December 3, 2020 12:44
@pashevskii
Copy link
Contributor Author

pashevskii commented Dec 3, 2020

Yes, you can wrap text and plus icon within <Button /> component, but not to forget to set plain prop on it.

I've added the plain button instead of the label and hidden all side effects in CSS (focus on the button).
image

@aleksfront
Copy link
Contributor

@pashevskii Left 3 more comments, ptal. Otherwise looks good.

aleksfront
aleksfront previously approved these changes Dec 4, 2020
@pashevskii pashevskii force-pushed the add-custom-repo branch 2 times, most recently from 93da7b0 to 2bb490d Compare December 7, 2020 06:06
Signed-off-by: Pavel Ashevskii <pashevskii@mirantis.com>
Copy link
Collaborator

@Nokel81 Nokel81 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Contributor

@aleksfront aleksfront left a comment

Choose a reason for hiding this comment

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

LGTM

@aleksfront aleksfront merged commit b7e5fb7 into lensapp:master Dec 8, 2020
@nevalla nevalla mentioned this pull request Dec 14, 2020
@jakolehm jakolehm mentioned this pull request Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants