feat: datepicker script setup #1972

Open
dpschen wants to merge 2 commits from dpschen/frontend:feat-datepicker-script-setup into main
Collaborator
There is no content yet.
dpschen added 1 commit 3 months ago
f10849dbfd
feat: datepicker script setup
konrad requested changes 3 months ago
konrad left a comment
Owner

Please fix the lint errors.

Please fix the lint errors.
Poster
Collaborator

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?
Poster
Collaborator

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.
Poster
Collaborator

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.
Poster
Collaborator

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 3 months ago
Poster
Collaborator

Changed to use the global i18n instance.

Changed to use the global i18n instance.
dpschen requested review from konrad 3 months ago
Collaborator

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 3 months ago
konrad requested changes 3 months ago
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 3 months ago
Poster
Collaborator

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`…
Poster
Collaborator

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 2 months ago
dpschen force-pushed feat-datepicker-script-setup from cf363aaf79 to b94ad289fc 2 months ago
konrad requested changes 1 month ago
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.
Poster
Collaborator

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 1 month ago
dpschen force-pushed feat-datepicker-script-setup from b94ad289fc to 6d40c13f4b 1 month ago
dpschen changed title from WIP: feat: datepicker script setup to feat: datepicker script setup 1 month ago
dpschen requested review from konrad 1 month ago
dpschen force-pushed feat-datepicker-script-setup from 6d40c13f4b to cd557f8c81 1 month ago
Poster
Collaborator

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 1 month ago
Poster
Collaborator

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 1 month ago
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)
chooseDateLabel: {
type: String as PropType<string>,
default() {
const {t} = useI18n()
konrad commented 1 month ago
Owner

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

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

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
})
function setDateValue(newVal: null | string | Date) {
console.log(newVal)
konrad commented 1 month ago
Owner

Please remove this.

Please remove this.
dpschen marked this conversation as resolved
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.
konrad commented 1 month ago
Owner

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
Poster
Collaborator

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 7 days ago
Poster
Collaborator

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 4 days ago
Poster
Collaborator

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 4 days ago
dpschen scheduled this pull request to auto merge when all checks succeed 4 days ago
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
Poster
Collaborator

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 :/

Reviewers

konrad was requested for review 4 days ago
Some checks failed
continuous-integration/drone/pr Build is failing
Required
Details
This Pull Request doesn't have enough approvals yet. 0 of 1 approvals granted.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#1972
Loading…
There is no content yet.