feat: move the calculation of the current salutation to a different function #1249

konrad · 2021-12-26T17:07:53Z

konrad commented

2021-12-26 17:07:53 +00:00

I hope to make the tests less flaky with that (it's almost always the home page test that fails).

konrad added 1 commit 2021-12-26 17:07:54 +00:00

feat: move the calculation of the current salutation to a different function

continuous-integration/drone/pr Build is passing

Details

173a16cc3b

konrad requested review from dpschen 2021-12-26 17:07:59 +00:00

frederick commented

2021-12-26 17:10:19 +00:00

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1249-featuremove-salutation--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi konrad! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1249-featuremove-salutation--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.

dpschen requested changes 2021-12-30 13:03:54 +00:00

src/helpers/hourToSalutation.ts Outdated

						
				@ -0,0 +1,21 @@

				export function hourToSalutation(now: Date = new Date()): String {

dpschen commented

2021-12-30 13:03:40 +00:00

Maybe we can return a translation id here instead of the final string. This way we can add support for translation

konrad commented

2021-12-30 16:15:01 +00:00

Done!

konrad marked this conversation as resolved

src/views/Home.vue Outdated

						
				@ -90,3 +71,1 @@

					return 'Night'

				})

				const welcome = computed(hourToSalutation)

dpschen commented

2021-12-30 13:02:25 +00:00

This computed will currently never change. My idea with the useNow was that when you keep the page open the title changes dynamically.

This computed will currently never change. My idea with the `useNow` was that when you keep the page open the title changes dynamically.

konrad commented

2021-12-30 16:20:07 +00:00

That makes sense.

I've reimplemented it with useNow and Date, that way we can put a controlled date in the function for the tests.

Not quite sure if this is the right way?

That makes sense. I've reimplemented it with `useNow` and `Date`, that way we can put a controlled date in the function for the tests. Not quite sure if this is the right way?

konrad marked this conversation as resolved

konrad added 1 commit 2021-12-30 16:14:49 +00:00

feat: return full translation key

continuous-integration/drone/pr Build is passing

Details

4a912a91d7

konrad added 1 commit 2021-12-30 16:19:02 +00:00

feat: use useNow to provide auto updates

continuous-integration/drone/pr Build is passing

Details

2c36356f3f

dpschen reviewed 2021-12-30 16:50:42 +00:00

src/helpers/hourToSalutation.ts Outdated

						
				@ -0,0 +3,4 @@

				const TRANSLATION_KEY_PREFIX = 'home.welcome'

				export function hourToSalutation(now: Date | Ref<Date> = useNow()): String {

dpschen commented

2021-12-30 16:37:39 +00:00

I'm quite unsure about this pattern now:

Normally our helpers are side effect free.
But this helper can contain a ref which has to be cleaned up.

It is almost a composable. Maybe we should go the full way and tranform it in one. Or would that make problems with testing?

OR

We keep the function (but remove the ref compatability) and add a composable.

Somehow like this (untested):

export function hourToSalutation(now = new Date): String {
	[...]
}

export function useDateTimeSalutation(now: MayBeRef<Date> = useNow()) {
	return computed(() => hourToSalutation(unref(now)))
}

EDIT: We could probably remove the parameter of useDateTimeSalutation because we don't need it (?).

I'm quite unsure about this pattern now: Normally our helpers are side effect free. But this helper can contain a ref which has to be cleaned up. It is almost a composable. Maybe we should go the full way and tranform it in one. Or would that make problems with testing? OR We keep the function (but remove the ref compatability) and add a composable. Somehow like this (untested): ```ts export function hourToSalutation(now = new Date): String { [...] } export function useDateTimeSalutation(now: MayBeRef<Date> = useNow()) { return computed(() => hourToSalutation(unref(now))) } ``` **EDIT**: We could probably remove the parameter of useDateTimeSalutation because we don't need it (?).

👍 1

dpschen commented

2021-12-30 16:41:03 +00:00

sidenote: vueuse has a cool type for this: MayBeRef

sidenote: vueuse has a cool type for this: [MayBeRef](https://github.com/vueuse/vueuse/blob/main/packages/shared/get/index.ts)

dpschen commented

2021-12-30 17:24:12 +00:00

Probably we can even remove the extra function. I thought it makes sense to keep it separated to make the function testable by itself.

But we can also just test the composable e.g. like vueuse does

Probably we can even remove the extra function. I thought it makes sense to keep it separated to make the function testable by itself. But we can also just test the composable e.g. like [vueuse does](https://github.com/antfu/vueuse/blob/main/packages/core/templateRef/index.test.ts)

dpschen commented

2022-01-04 16:57:48 +00:00

I changed it to this comment now to keep it simple.

We can still change the composable tests to something similar than vueuse does later.

I changed it to [this comment](https://kolaente.dev/vikunja/frontend/pulls/1249#issuecomment-22782) now to keep it simple. We can still change the composable tests to something similar than vueuse does later.

src/helpers/hourToSalutation.ts Outdated

						
				@ -0,0 +4,4 @@

				const TRANSLATION_KEY_PREFIX = 'home.welcome'

				export function hourToSalutation(now: Date | Ref<Date> = useNow()): String {

					const hours = now instanceof Date ? new Date(now).getHours() : new Date(now.value).getHours()