feat: simplify heading blur logic #727

dpschen · 2021-09-08T21:44:10Z

dpschen commented

2021-09-08 21:44:10 +00:00

I saw the recent change with the the blur in the heading and remembered I had something like that recently. With this trick there is always just one call.

dpschen reviewed 2021-09-08 21:47:03 +00:00

src/components/tasks/partials/heading.vue Outdated

						
				@ -66,4 +60,1 @@

							// Pull the task title from the contenteditable

							let taskTitle = this.$refs.taskTitle.textContent

							this.task.title = taskTitle

dpschen commented

2021-09-08 21:47:03 +00:00

By assigning taskTitle to this.task.title the task is getting modified.
I guess this is the only reason why a local copy of the task existed. By getting the value as a parameter from the save function there is no need to modify the task directly.

By assigning `taskTitle` to `this.task.title` the `task` is getting modified. I guess this is the only reason why a local copy of the task existed. By getting the value as a parameter from the `save` function there is no need to modify the task directly.

konrad commented

2021-09-08 21:55:51 +00:00

I'm pretty sure there's a few cases like this in other places.

dpschen commented

2021-09-08 22:03:03 +00:00

Yes! Those are the ones that give me headache with vue3 :)

😆 1

dpschen marked this conversation as resolved

dpschen reviewed 2021-09-08 21:47:58 +00:00

src/components/tasks/partials/heading.vue Outdated

						
				@ -12,2 +10,3 @@

							@blur="save($event.target.textContent)"

							@keydown.enter.prevent.stop="$event.target.blur()"

							:contenteditable="canWrite ? 'true' : 'false'"

							ref="taskTitle">{{ task.title.trim() }}</h1>

dpschen commented

2021-09-08 21:47:58 +00:00

By trimming the title on saving (see in save function) there should never be a title with whitespace around.
Maybe there are old ones in the database but they should disappear :)

By trimming the title on saving (see in `save` function) there should never be a title with whitespace around. Maybe there are old ones in the database but they should disappear :)

konrad commented

2021-09-08 21:53:18 +00:00

I agree in principle, but remember the api can theoretically be used with other clients as well. Therefore we cannot be sure there won't be any spaces around the task title without the trim(). Unless we add a trim on task create/update in the api. That might actually be a good idea.

I agree in principle, but remember the api _can_ theoretically be used with other clients as well. Therefore we cannot be sure there won't be any spaces around the task title without the `trim()`. Unless we add a trim on task create/update in the api. That might actually be a good idea.

👍 1

dpschen commented

2021-09-08 22:03:59 +00:00

How about keeping both?

konrad commented

2021-09-08 22:04:59 +00:00

You mean on update/create and when displaying it? Sounds great. But then the trim on create/update should go into the TaskService.

👍 1

dpschen commented

2021-09-08 22:32:10 +00:00

Good point.
Not sure where exactly to put this then in the TaskService.
I had already problems with the processModel method, because it modifies the original task in the store instead of creating a new one. I guess it would be something like:

// NOTE: not sure if a task is able to have an empty string.
model.title = model.title.trim() || ''

Good point. Not sure where exactly to put this then in the TaskService. I had already problems with the `processModel` method, because it modifies the original task in the store instead of creating a new one. I guess it would be something like: ```js // NOTE: not sure if a task is able to have an empty string. model.title = model.title.trim() || '' ```

dpschen commented

2021-09-10 12:46:16 +00:00

You mean on update/create and when displaying it? Sounds great. But then the trim on create/update should go into the TaskService.

Okay, I put this now in the TaskService processModel method.
I'm just a bit confused because it seems to me that almost the same stuff is done in the constructor of the TaskModel, so I added it there aswell.
Shouldn't those two operation be identical / happen just once?
Or what is the difference?

> You mean on update/create and when displaying it? Sounds great. But then the trim on create/update should go into the TaskService. Okay, I put this now in the TaskService `processModel` method. I'm just a bit confused because it seems to me that almost the same stuff is done in the constructor of the TaskModel, so I added it there aswell. Shouldn't those two operation be identical / happen just once? Or what is the difference?

konrad commented

2021-09-10 12:56:07 +00:00

It's almost the same, but not quite. processModel is called before a model is sent to the api, the task constructor is called when a model is returned from the api. They are mostly used to convert js date objects to iso dates and create releated models and such.

I'm sure there are better ways to do this.

Oh and by the way, check your email 🙂

It's almost the same, but not quite. `processModel` is called before a model is sent to the api, the task constructor is called when a model is returned from the api. They are mostly used to convert js date objects to iso dates and create releated models and such. I'm sure there are better ways to do this. Oh and by the way, check your email 🙂

dpschen commented

2021-09-10 13:20:34 +00:00

the task constructor is called when a model is returned from the api.

So checking if I understood this right:

the processModel functions in the services are some kind of validation / formatting so that the data has the right format for the api.
the constructors of the models are intended to make some changes to make it easier to work with the data on client side.

If that's correct I don't understand why new XYModel (aka calling the model constructor) is called in so many places where there is no data coming from the api.

Can you point me to an example where the constructor is called when e.g. the task model is returned from the api?

Oh and by the way, check your email 🙂

Checking!

> the task constructor is called when a model is returned from the api. So checking if I understood this right: - the `processModel` functions in the services are some kind of validation / formatting so that the data has the right format for the api. - the constructors of the models are intended to make some changes to make it easier to work with the data on client side. If that's correct I don't understand why `new XYModel` (aka calling the model constructor) is called in so many places where there is no data coming from the api. Can you point me to an example where the constructor is called when e.g. the task model is returned from the api? > Oh and by the way, check your email 🙂 Checking!

konrad commented

2021-09-10 14:11:46 +00:00

If that's correct I don't understand why new XYModel (aka calling the model constructor) is called in so many places where there is no data coming from the api.

The services have a modelFactory function which receives data from the api and returns a new XYZModel(data). The other cases where a new model is created is when creating these things, for example a new task. That might be a bit redundant though. Again, I'm sure there are better way to do this.
I created the whole Services / Model implementation based on some things I did in other languages or that I've seen with not much experience.

> If that's correct I don't understand why new XYModel (aka calling the model constructor) is called in so many places where there is no data coming from the api. The services have a `modelFactory` function which receives data from the api and returns a `new XYZModel(data)`. The other cases where a new model is created is when creating these things, for example a new task. That might be a bit redundant though. Again, I'm sure there are better way to do this. I created the whole Services / Model implementation based on some things I did in other languages or that I've seen with not much experience.

👍 1

dpschen commented

2021-09-10 16:47:34 +00:00

Sure, just want to make sure that I understand the imagined role of the parts :)

dpschen reviewed 2021-09-08 21:48:42 +00:00

src/components/tasks/partials/heading.vue Outdated

						
				@ -17,3 +18,3 @@

								{{ $t('misc.saving') }}

							</span>

							<span class="has-text-success is-inline-flex is-align-content-center" v-if="!loading && saved">

							<span class="has-text-success is-inline-flex is-align-content-center" v-if="!loading && showSavedMessage">

dpschen commented

2021-09-08 21:48:42 +00:00

I renamed this variable to make the meaning more clear.

👍 1

dpschen marked this conversation as resolved

dpschen reviewed 2021-09-08 21:54:09 +00:00

src/components/tasks/partials/heading.vue Outdated

						
				@ -25,22 +26,25 @@

				</template>

				<script>

				import {LOADING} from '@/store/mutation-types'

dpschen commented

2021-09-08 21:54:09 +00:00

Since LOADING is used here to get a state via mapState and (and not mutation-type via mapMutation) this seemed like a mistake.

Since `LOADING` is used here to get a state via `mapState` and (and not mutation-type via `mapMutation`) this seemed like a mistake.

👍 1

dpschen marked this conversation as resolved

konrad reviewed 2021-09-08 21:56:04 +00:00

src/components/tasks/partials/heading.vue Outdated

						
				@ -10,2 +10,2 @@

							@focusout="save()"

							@keydown.enter.prevent.stop="save()"

							@blur="save($event.target.textContent)"

							@keydown.enter.prevent.stop="$event.target.blur()"

konrad commented

2021-09-08 21:51:33 +00:00

Wow, that's as elegant as it is obvious.

dpschen commented

2021-09-08 22:09:45 +00:00

Thanks :) Was also new to me. Google helped :D
Before that i always used something like _.throttle ; but that seemed too overpowered here.

Thanks :) Was also new to me. Google helped :D Before that i always used something like _.throttle ; but that seemed too overpowered here.

konrad commented

2021-09-08 22:11:35 +00:00

I've used good ol' setTimeout in the past but that never felt good. It didn't even work all the time...

I've used good ol' `setTimeout` in the past but that never felt good. It didn't even work all the time...

dpschen commented

2021-09-08 22:20:04 +00:00

Yes :D
When you use setTimeout it's almost (!) always a hint that you are creating a workaround for something and not using the "right" ™️ way.

Yes :D When you use setTimeout it's almost (!) always a hint that you are creating a workaround for something and not using the "right" ™️ way.

konrad marked this conversation as resolved

dpschen reviewed 2021-09-08 21:58:55 +00:00

src/components/tasks/partials/heading.vue Outdated

						
				@ -92,3 +73,1 @@

								.then(() => {

									this.$emit('input', this.task)

									this.saved = true

							const newTask = {

dpschen commented

2021-09-08 21:58:55 +00:00

We created a new task to prevent unwanted mutations

konrad marked this conversation as resolved

dpschen reviewed 2021-09-08 22:01:12 +00:00

src/components/tasks/partials/heading.vue Outdated

						
				@ -62,4 +57,1 @@

					},

					methods: {

						save() {

							this.$refs.taskTitle.spellcheck = false

dpschen commented

2021-09-08 22:01:12 +00:00

I was not sure why spellcheck is set to false here.
Also it's never set to undefined / false later.

I removed the direct element access and added a default value. Maybe wrong assumptions?

I was not sure why spellcheck is set to `false` here. Also it's never set to `undefined` / `false` later. I removed the direct element access and added a default value. Maybe wrong assumptions?

konrad commented

2021-09-08 22:03:09 +00:00

Probably because I wanted to disable it before saving. What does the commit message on that line say?

dpschen commented

2021-09-08 22:10:21 +00:00

It was from the original commit of that component.

It was from the [original commit]( https://kolaente.dev/vikunja/frontend/commit/148cc1dcca864032304a42d59088c48ce70dbc8c) of that component.

konrad commented

2021-09-08 22:14:40 +00:00

I see. I think it makes sense to just always disable spellcheck, but I'm pretty sure I tried to do that back then and got bitten by it somehow.

dpschen commented

2021-09-08 22:23:02 +00:00