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.

Sign up for GitHub

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

Jump to bottom

feat(ui): Change default language for Resource Editor to YAML and store preference in localStorage. Fixes #3543 #3560

Merged
merged 14 commits into from
Jul 23, 2020
Merged

feat(ui): Change default language for Resource Editor to YAML and store preference in localStorage. Fixes #3543 #3560

merged 14 commits into from
Jul 23, 2020

Conversation

rbreeze
Copy link
Member

@rbreeze rbreeze commented Jul 22, 2020 •
edited
Loading

Also makes note about auto-completion more prominent:
Screen Shot 2020-07-22 at 4 44 35 PM

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed the CLA.
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

Fixes #3543

@rbreeze
Change default language in resource editor to YAML. Make note about a…
eb78237 
…uto-completion more prominent. Store preference in localStorage.
@rbreeze rbreeze changed the title enhancement(ui): Change default language for Resource Editor to YAML and store preference in localStorage enhancement(ui): Change default language for Resource Editor to YAML and store preference in localStorage. Fixes #3543 Jul 22, 2020
@rbreeze rbreeze mentioned this pull request Jul 22, 2020
Closed
@rbreeze rbreeze changed the title enhancement(ui): Change default language for Resource Editor to YAML and store preference in localStorage. Fixes #3543 feat(ui): Change default language for Resource Editor to YAML and store preference in localStorage. Fixes #3543 Jul 22, 2020
@alexec
Copy link
Contributor

alexec commented Jul 23, 2020

Thank you. I think we can keep the note as a footer. I think the text could just be.

"Basic completion for YAML. Switch to JSON for full auto-completion"

The YAML button has been bugging me for ages - it looks bad. Can we replace the YAML buttons check box with <i className='fa fa-checked'/> (only when checked obviously. I think it looks better - but I leave it to your discretion how to improve it. Don't spend more than an hour on it naturally.

@rbreeze
Copy link
Member Author

rbreeze commented Jul 23, 2020

Here's what I came up with. I also tweaked the editor container itself, which I think may fix #3542 as well (will need to confirm).

Screen Shot 2020-07-23 at 10 07 49 AM

The font was also set to Courier / monospace which wasn't being applied to the code itself but rather the right click menu, so I changed that as well:

Screen Shot 2020-07-23 at 10 04 58 AM

@alexec
Copy link
Contributor

alexec commented Jul 23, 2020

perfect

@alexec alexec self-assigned this Jul 23, 2020
alexec
alexec reviewed Jul 23, 2020
View reviewed changes
ui/src/app/shared/components/resource-editor/resource-editor.tsx
private static getLang(): string {
const stored = localStorage.getItem(LOCAL_STORAGE_KEY);
if (stored !== null) {
if (stored === 'yaml' || stored === 'json') {
Copy link
Contributor

Choose a reason for hiding this comment

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

good guard condition here

alexec
alexec requested changes Jul 23, 2020
View reviewed changes
ui/src/app/shared/components/resource-editor/resource-editor.tsx Outdated
}

private set type(type: string) {
this.setState({type, value: stringify(parse(this.state.value), type)});
private static getLang(): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe loadLang?

ui/src/app/shared/components/resource-editor/resource.scss Outdated
}

&__lang-toggle {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we avoid creating a new style like this - I think the existing "-o" style is acceptable

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. The margin right was to address the inconsistent spacing as shown below:

Screen Shot 2020-07-23 at 2 33 35 PM

and the width style was to avoid the button changing sizes each time the user clicks it. Would it be a good idea to inline those instead? I'll remove the &.yaml styles.

Copy link
Contributor

Choose a reason for hiding this comment

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

could you use a different (empty?) icon for unchecked?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't love how the empty icon for unchecked looked so I used an "x" instead. Does the job

@rbreeze rbreeze merged commit 1b757ea into argoproj:master Jul 23, 2020
@rbreeze rbreeze deleted the yaml-default branch July 23, 2020 23:47
@rbreeze rbreeze mentioned this pull request Jul 24, 2020
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexec alexec alexec approved these changes

Assignees

@alexec alexec

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New UI editor should default to YAML
2 participants
@rbreeze @alexec