Skip to content

Use v-model for inputs #224

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

Merged
merged 5 commits into from
Jun 27, 2021
Merged

Conversation

FuriouZz
Copy link
Contributor

@FuriouZz FuriouZz commented Jun 19, 2021

Use v-model logic.
Issue #190


This change is Reviewable

@TrueDoctor TrueDoctor requested a review from Keavon June 19, 2021 16:48
Copy link
Member

@Keavon Keavon left a comment

Choose a reason for hiding this comment

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

Thanks for this! Would you also please also add this v-model reactivity to NumberInput.vue? We will need to make use of the number input widget very soon for a couple new features that the backend is building. This should probably be updated whenever the user hits enter (which should remove focus from text entry) or when the user clicks out of the input box (and therefore removes focus from the text entry). If you rebase onto the current master, I fixed it so it is now possible again to type numbers into the input box.

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @FuriouZz)


client/web/src/components/widgets/floating-menus/FloatingMenu.vue, line 354 at r1 (raw file):

	},
	computed: {
		floatingMenuContentStyle(): unknown {

Thanks for this change! It also reminded me that you made your excellent feedback on my PR shortly after I submitted it, and I don't think I addressed that. I'm going to open it up so I don't forget again.

Is it possible to avoid the unknown type? For example, can we use a { minWidth: string } type, or define an interface?


client/web/src/components/widgets/inputs/DropdownInput.vue, line 8 at r1 (raw file):

			<Icon :class="'dropdown-arrow'" :icon="'DropdownArrow'" />
		</div>
		<MenuList :menuEntries="menuEntries" v-model:active-entry="activeEntry" :direction="MenuDirection.Bottom" @width-changed="onWidthChanged" :drawIcon="drawIcon" ref="menuList" />

Question (not requesting a change): what should determine whether a property is camelCase or kebab-case? I see you changed activeEntry to v-model:active-entry and :widthChanged to @width-changed. Is it everything except v-bind:foo (shorthand :foo) items? I want to learn the pattern for what I presume is the best practice in Vue so I can follow it from now on if I can remember to.

@FuriouZz
Copy link
Contributor Author

Thanks for this! Would you also please also add this v-model reactivity to NumberInput.vue? We will need to make use of the number input widget very soon for a couple new features that the backend is building. This should probably be updated whenever the user hits enter (which should remove focus from text entry) or when the user clicks out of the input box (and therefore removes focus from the text entry).

You right, I must watch on every inputs. We have to see every input as a HTML Input and use v-model on it.

Thanks for this change! It also reminded me that you made your excellent feedback on my PR shortly after I submitted it, and I don't think I addressed that. I'm going to open it up so I don't forget again.

Is it possible to avoid the unknown type? For example, can we use a { minWidth: string } type, or define an interface?

Of course

Question (not requesting a change): what should determine whether a property is camelCase or kebab-case? I see you changed activeEntry to v-model:active-entry and :widthChanged to @width-changed. Is it everything except v-bind:foo (shorthand :foo) items? I want to learn the pattern for what I presume is the best practice in Vue so I can follow it from now on if I can remember to.

@Keavon I used kebab-case for props because this is a style recommandation from Vue (link). In HTML, it is more common to use kebab-case, where as in Javascript camelCase. Vue do the translation for you, it's not a problem. If you were using JSX, camelCase for props would be prefered.

@FuriouZz
Copy link
Contributor Author

I saw that we cannot use shortcuts for development case (opening inspector or refresh the tab). It could be cool to allow these shortcuts in development environment for ease.

Copy link
Member

@Keavon Keavon left a comment

Choose a reason for hiding this comment

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

Please rebase, I just fixed that! We can use F11 (fullscreen), F12 (open dev tools), Ctrl+Shift+C (select element in DOM), and type numbers into the text input fields again.

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @FuriouZz)

Copy link
Member

@Keavon Keavon left a comment

Choose a reason for hiding this comment

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

Does this also mean we should update all our :fooBar into :foo-bar throughout the app?

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @FuriouZz)

Copy link
Contributor Author

@FuriouZz FuriouZz left a comment

Choose a reason for hiding this comment

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

Yes, it will be better according to Vue Style Guide. It could be good that our frontend style guide stick to Vue Style Guide too.

Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @FuriouZz)

Copy link
Contributor Author

@FuriouZz FuriouZz left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @FuriouZz and @Keavon)


client/web/src/components/widgets/floating-menus/FloatingMenu.vue, line 354 at r1 (raw file):

Previously, Keavon (Keavon Chambers) wrote…

Thanks for this change! It also reminded me that you made your excellent feedback on my PR shortly after I submitted it, and I don't think I addressed that. I'm going to open it up so I don't forget again.

Is it possible to avoid the unknown type? For example, can we use a { minWidth: string } type, or define an interface?

I used CSSStyleDeclaration

Copy link
Contributor Author

@FuriouZz FuriouZz left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 2 unresolved discussions (waiting on @FuriouZz and @Keavon)


client/web/src/components/widgets/floating-menus/FloatingMenu.vue, line 354 at r1 (raw file):

Previously, FuriouZz (Chrs Msln) wrote…

I used CSSStyleDeclaration

Done.

@FuriouZz FuriouZz requested a review from Keavon June 20, 2021 12:08
@@ -1,8 +1,8 @@
<template>
Copy link
Member

Choose a reason for hiding this comment

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

Would you have time to modify this so it behaves such that: you click in the center (not the left or right arrow) and it puts you into text entry mode (while the is focused) with a slightly lighter background color, and the unit is removed from the number, the left/right arrows are hidden, text entry is left-aligned, and all the text starts highlighted so it's easy to replace the whole number, and finally when you hit enter or click out of the component, it applies the number and restores the styling including the unit reappears, and the updated number is only then sent to the user of the component.

@Keavon Keavon linked an issue Jun 24, 2021 that may be closed by this pull request
@Keavon
Copy link
Member

Keavon commented Jun 24, 2021

Hi @FuriouZz, I just wanted to check that you saw my previous comment. If you don't have time to add that functionality, I can do it over the next few days. It'll be needed for setting opacity which @geom3trik is working on.

@Keavon
Copy link
Member

Keavon commented Jun 27, 2021

I'll go ahead and merge this and one of us can implement the functionality I described above.

@Keavon Keavon merged commit b429913 into GraphiteEditor:master Jun 27, 2021
@FuriouZz FuriouZz deleted the feat/use-v-model branch July 2, 2021 09:03
Keavon pushed a commit that referenced this pull request Jun 16, 2022
* Use v-model for inputs

* Add opacity to LayerTree

* Fix FloatingMenu typing
Keavon pushed a commit that referenced this pull request Jul 30, 2023
* Use v-model for inputs

* Add opacity to LayerTree

* Fix FloatingMenu typing
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.

Refactor Vue input components with v-model to support input and output
2 participants