WIP: REFERENCE datepicker script setup alternative #1972

Closed
dpschen wants to merge 1 commits from dpschen/frontend:feat-datepicker-script-setup into main
Member
No description provided.
dpschen added 1 commit 2022-05-20 22:53:32 +00:00
continuous-integration/drone/pr Build is failing Details
f10849dbfd
feat: datepicker script setup
konrad requested changes 2022-05-22 14:49:23 +00:00
konrad left a comment
Owner

Please fix the lint errors.

Please fix the lint errors.
Author
Member

I am actually not sure how to solve this correctly:

<script setup lang="ts>
// t will not be available when props are checked because defineProps is extracted from setup
const {t} = useI18n()


const props = defineProps({

	},
	chooseDateLabel: {
		type: String,
		default: () => t('input.datepicker.chooseDate'),
	},
})

What I can do is to import the global i18n instance. Not sure that would be the way one is used to do this 🤔

Generally i18n annoys me. With the V9 version the way you are supposed to work is that you have local messages in every component…

I am actually not sure how to solve this correctly: ```ts <script setup lang="ts> // t will not be available when props are checked because defineProps is extracted from setup const {t} = useI18n() const props = defineProps({ }, chooseDateLabel: { type: String, default: () => t('input.datepicker.chooseDate'), }, }) ``` What I can do is to import the global i18n instance. Not sure that would be the way one is used to do this 🤔 Generally i18n annoys me. With the V9 version the way you are supposed to work is that you have local messages in every component…
Owner

What about making chooseDateLabel a computed and defining the default value in there?

What about making `chooseDateLabel` a computed and defining the default value in there?
Author
Member

That's possible.

I just think it's a bit annoying because using props and falling back to default values is kind of "the vue way to do things" :D

Personally I think importing the i18n instance is the best way for now, but maybe I'll create an issue in the vue-i18n-next repo.

That's possible. I just think it's a bit annoying because using props and falling back to default values is kind of "the vue way to do things" :D Personally I think importing the i18n instance is the best way for now, but maybe I'll create an issue in the vue-i18n-next repo.
Author
Member

Are you fine with using the instance here?

Are you fine with using the instance here?
Owner

Are you fine with using the instance here?

I think that's okay, yes. Please follow up with an issue in the i18n repo though.

> Are you fine with using the instance here? I think that's okay, yes. Please follow up with an issue in the i18n repo though.
Author
Member

Question in vue-i18n-next discussion (author answers frequently):
https://github.com/intlify/vue-i18n-next/discussions/1012

Question in vue-i18n-next discussion (author answers frequently): https://github.com/intlify/vue-i18n-next/discussions/1012
dpschen force-pushed feat-datepicker-script-setup from f10849dbfd to 7fb5741615 2022-05-22 15:30:42 +00:00 Compare
Author
Member

Changed to use the global i18n instance.

Changed to use the global i18n instance.
dpschen requested review from konrad 2022-05-22 15:31:16 +00:00
Member

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1972-feat-datepicker-script-setup--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 dpschen! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1972-feat-datepicker-script-setup--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 force-pushed feat-datepicker-script-setup from 7fb5741615 to ce65adca2a 2022-05-22 17:13:11 +00:00 Compare
konrad requested changes 2022-05-22 21:06:56 +00:00
konrad left a comment
Owner

Looks like the label now does not work at all:

image

Looks like the label now does not work at all: ![image](/attachments/3ae21295-e012-4068-8072-1fa5cf69081e)
3.1 KiB
dpschen force-pushed feat-datepicker-script-setup from ce65adca2a to cf363aaf79 2022-05-22 21:41:36 +00:00 Compare
Author
Member

The problem was not vue-i18n but that the label didn't get displayed because we compared it with null. But I changed to type of the ref to Date | undefined

The problem was not vue-i18n but that the label didn't get displayed because we compared it with `null`. But I changed to type of the ref to `Date | undefined`…
Author
Member

Looks like the label now does not work at all:

image

@konrad This was fixed. Anything else missing here?

> Looks like the label now does not work at all: > > ![image](/attachments/3ae21295-e012-4068-8072-1fa5cf69081e) @konrad This was fixed. Anything else missing here?
dpschen requested review from konrad 2022-06-21 22:37:21 +00:00
dpschen force-pushed feat-datepicker-script-setup from cf363aaf79 to b94ad289fc 2022-06-21 22:38:26 +00:00 Compare
konrad requested changes 2022-07-04 15:51:17 +00:00
konrad left a comment
Owner

It looks like when I click on "confirm" it will close the popup but not set a date.

It looks like when I click on "confirm" it will close the popup but not set a date.
Author
Member

Will check again. Thanks for testing

Will check again. Thanks for testing
dpschen changed title from feat: datepicker script setup to WIP: feat: datepicker script setup 2022-07-04 22:02:17 +00:00
dpschen force-pushed feat-datepicker-script-setup from b94ad289fc to 6d40c13f4b 2022-07-10 13:29:25 +00:00 Compare
dpschen changed title from WIP: feat: datepicker script setup to feat: datepicker script setup 2022-07-10 13:30:23 +00:00
dpschen requested review from konrad 2022-07-10 13:30:27 +00:00
dpschen force-pushed feat-datepicker-script-setup from 6d40c13f4b to cd557f8c81 2022-07-10 13:32:58 +00:00 Compare
Author
Member

This should work now. I updated the code a bit so that now is live. Meaning that if you open the popup on 23:59 and the time changes to the next day the values stay up-to-date.

This should work now. I updated the code a bit so that now is live. Meaning that if you open the popup on 23:59 and the time changes to the next day the values stay up-to-date.
konrad was assigned by dpschen 2022-07-10 13:33:50 +00:00
Author
Member

I kill the CI though because of recent problems. Please retrigger, when if runs again :)

I kill the CI though because of recent problems. Please retrigger, when if runs again :)
konrad requested changes 2022-07-11 10:03:46 +00:00
konrad left a comment
Owner

When adding a new reminder I get a vuex mutation error:

image

When adding a new reminder I get a vuex mutation error: ![image](/attachments/7a62159f-52c8-408e-b45b-218ce66a89d6)
@ -132,0 +67,4 @@
chooseDateLabel: {
type: String as PropType<string>,
default() {
const {t} = useI18n()

Why don't you add the useI18n before the props declaration?

Why don't you add the `useI18n` before the props declaration?
Author
Member

That wouldn't work. Because afaik the props, events declaration and imports are extracted from script setup and the rest will be wrapped by the setup function. Meaning that if I would use use18n before defineProps it would still be inside the setup block after beeing compiled => no access inside the props.

That wouldn't work. Because afaik the props, events declaration and imports are extracted from script setup and the rest will be wrapped by the setup function. Meaning that if I would use `use18n` before `defineProps` it would still be inside the setup block after beeing compiled => no access inside the props.
dpschen marked this conversation as resolved
@ -239,1 +167,4 @@
})
function setDateValue(newVal: null | string | Date) {
console.log(newVal)

Please remove this.

Please remove this.
dpschen marked this conversation as resolved
@ -240,0 +199,4 @@
function close() {
// Kind of dirty, but the timeout allows us to enter a time and click on "confirm" without
// having to click on another input field before it is actually used.

I think this doesn't actually work? But that doesn't really work right now either, more of a mental note to properly fix this some time.

I think this doesn't actually work? But that doesn't really work right now either, more of a mental note to properly fix this some time.
dpschen marked this conversation as resolved
Author
Member

When adding a new reminder I get a vuex mutation error:

image

This should probably also happen with the current version.
This PR is again mostly about converting to script setup.
I didn't want to change the logic too much – I would change a lot :)

> When adding a new reminder I get a vuex mutation error: > > ![image](/attachments/7a62159f-52c8-408e-b45b-218ce66a89d6) This should probably also happen with the current version. This PR is again mostly about converting to script setup. I didn't want to change the logic too much – I would change a lot :)
Owner

This should probably also happen with the current version.

As far as I tested it, it does not happen on the main branch 🤔

> This should probably also happen with the current version. As far as I tested it, it does not happen on the main branch 🤔
dpschen force-pushed feat-datepicker-script-setup from cd557f8c81 to 80e44741a7 2022-08-05 17:41:29 +00:00 Compare
Author
Member

This should probably also happen with the current version.

As far as I tested it, it does not happen on the main branch 🤔

I just tested this again. I can't reproduce the error that you have.
When does this happen to you?

> > This should probably also happen with the current version. > > As far as I tested it, it does not happen on the main branch 🤔 I just tested this again. I can't reproduce the error that you have. When does this happen to you?
Owner

Looks like the vuex error is gone.

But the styling is broken now:

image

Current main branch:

image

Looks like the vuex error is gone. But the styling is broken now: ![image](/attachments/834a89ec-f81b-4a26-9e38-b5b4fe293411) Current main branch: ![image](/attachments/fb70041b-92f1-41d2-b61f-8c245a8fbd28)
dpschen force-pushed feat-datepicker-script-setup from 80e44741a7 to 43b66d5759 2022-08-08 21:43:50 +00:00 Compare
Author
Member

Should be fixed. I think that would also have been resolved by rebasing :D

Should be fixed. I think that would also have been resolved by rebasing :D
dpschen requested review from konrad 2022-08-08 21:44:56 +00:00
dpschen scheduled this pull request to auto merge when all checks succeed 2022-08-08 21:45:02 +00:00
Owner

Now the button to add another reminder disappeared:

image

Now the button to add another reminder disappeared: ![image](/attachments/99e727ba-e01d-4ca2-a694-574b1f9e7b40)
5.9 KiB
Author
Member

Now the button to add another reminder disappeared:

image

The style entanglement is so crazy :/

> Now the button to add another reminder disappeared: > > ![image](/attachments/99e727ba-e01d-4ca2-a694-574b1f9e7b40) The style entanglement is so crazy :/
dpschen force-pushed feat-datepicker-script-setup from 43b66d5759 to 8612ca00b2 2022-08-19 09:23:31 +00:00 Compare
dpschen force-pushed feat-datepicker-script-setup from 8612ca00b2 to b87436c35c 2022-08-22 13:49:55 +00:00 Compare
Owner

Now the button to add another reminder disappeared:

Looks like this is still a problem.

> Now the button to add another reminder disappeared: Looks like this is still a problem.
Author
Member

Yes. I couldn't find the underlying issue yet.

Yes. I couldn't find the underlying issue yet.
dpschen changed title from feat: datepicker script setup to WIP: feat: datepicker script setup 2022-10-03 16:18:56 +00:00
Author
Member

Keeping this as reference. We do have one version merged now, but that was just a simple translation. In here are some nice ideas for improvement that I want to port over.

Keeping this as reference. We do have one version merged now, but that was just a simple translation. In here are some nice ideas for improvement that I want to port over.
dpschen changed title from WIP: feat: datepicker script setup to WIP: ARCHIVE datepicker script setup alternative 2022-10-31 19:20:56 +00:00
dpschen changed title from WIP: ARCHIVE datepicker script setup alternative to WIP: REFERENCE datepicker script setup alternative 2022-11-04 12:19:31 +00:00
Owner

Closing this as it appears stale.

Please rebase and reopen if you think this is still relevant.

Closing this as it appears stale. Please rebase and reopen if you think this is still relevant.
konrad closed this pull request 2023-10-22 17:43:58 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.