vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

fix: add interval to uses of useNow so that it uses less resources #3255

Merged
dpschen merged 6 commits from feature/add-interval-to-use-now into main 2023-03-24 23:30:29 +00:00
Conversation 4 Commits 6 Files Changed 3 +10 -7
konrad commented 2023-03-16 18:25:46 +00:00
Owner
Copy Link

Resolves vikunja/api#1434

Resolves https://kolaente.dev/vikunja/api/issues/1434
konrad added 1 commit 2023-03-16 18:25:46 +00:00
konrad requested review from dpschen 2023-03-16 18:25:52 +00:00
konrad commented 2023-03-16 18:26:25 +00:00
Author
Owner
Copy Link

@WofWca can you check if this fixes it?

@WofWca can you check if this fixes it?
frederick commented 2023-03-16 18:30:22 +00:00
Member
Copy Link

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3255-feature-add-interval-to-use-now--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://3255-feature-add-interval-to-use-now--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 commented 2023-03-16 20:29:18 +00:00
Member
Copy Link

Shouldn't the speed impact be solved by using an interval of a second? This way the time is almost up to date. The time to calculate should be neglectible. Regardless: requestanimationframe is supposed to be quite efficient. It would interest me why this would have a large impact on our performance.

Shouldn't the speed impact be solved by using an interval of a second? This way the time is almost up to date. The time to calculate should be neglectible. Regardless: `requestanimationframe` is supposed to be quite efficient. It would interest me why this would have a large impact on our performance.
dpschen reviewed 2023-03-16 20:29:38 +00:00
src/components/home/Logo.vue
@ -7,2 +7,3 @@
const now = useNow()
const now = useNow({
interval: 24 * 60 * 60 * 1000, // Every day
dpschen commented 2023-03-16 20:22:34 +00:00
Member

We want that the Logo changes at midnight, correct? By setting this to change in an interval of a day this might happen at 23:00 the next day if the logo page with the logo was opened at that time the day before. Could we configure this somehow so that it's triggered every real daychange? Or hour change?

We want that the Logo changes at midnight, correct? By setting this to change in an interval of a day this might happen at 23:00 the next day if the logo page with the logo was opened at that time the day before. Could we configure this somehow so that it's triggered every real daychange? Or hour change?
WofWca commented 2023-03-17 10:55:03 +00:00
Contributor

Calculating how much time left until the point of change + setTimeout (possibly setInterval in case the page is not reloaded for months) should work.

Calculating how much time left until the point of change + `setTimeout` (possibly `setInterval` in case the page is not reloaded for months) should work.
konrad commented 2023-03-23 21:14:35 +00:00
Author
Owner

Ideally we want to change the logo at midnight, yes. But since this is not something I'd consider critical and it will be shown for a whole month it should be enough to check this once a day. Even if that means it might lag behind one day.

Ideally we want to change the logo at midnight, yes. But since this is not something I'd consider critical and it will be shown for a whole month it should be enough to check this once a day. Even if that means it might lag behind one day.
dpschen commented 2023-03-24 19:00:27 +00:00
Member

We should use the time unit constants for this.

How about using every hour? One tick per hour should really not create any performance issues (as said earlier I think once per second should also not be an issue.

We should use the time unit constants for this. How about using every hour? One tick per hour should really not create any performance issues (as said earlier I think once per second should also not be an issue.
dpschen commented 2023-03-24 19:00:28 +00:00
Member

We should use the time unit constants for this.

How about using every hour? One tick per hour should really not create any performance issues (as said earlier I think once per second should also not be an issue.

We should use the time unit constants for this. How about using every hour? One tick per hour should really not create any performance issues (as said earlier I think once per second should also not be an issue.
konrad commented 2023-03-24 19:10:15 +00:00
Author
Owner

We should use the time unit constants for this.

Done!

> We should use the time unit constants for this. Done!
👍 1
konrad commented 2023-03-24 19:10:57 +00:00
Author
Owner

How about using every hour? One tick per hour should really not create any performance issues (as said earlier I think once per second should also not be an issue.

and done!

> How about using every hour? One tick per hour should really not create any performance issues (as said earlier I think once per second should also not be an issue. and done!
👍 1
konrad marked this conversation as resolved
src/components/tasks/GanttChart.vue Outdated
@ -159,2 +159,3 @@
const today = useNow()
const today = useNow({
interval: 6 * 60 * 60 * 1000, // Every 6 hours
dpschen commented 2023-03-16 20:23:37 +00:00
Member

If we don't change this constantly I think it would be better to only change it when the user switches view. Changing it every 6 hours seems random in comparison.

If we don't change this constantly I think it would be better to only change it when the user switches view. Changing it every 6 hours seems random in comparison.
👍 1
konrad commented 2023-03-23 21:15:09 +00:00
Author
Owner

If we don't change this constantly I think it would be better to only change it when the user switches view.

That's a good point.

> If we don't change this constantly I think it would be better to only change it when the user switches view. That's a good point.
konrad commented 2023-03-24 18:49:08 +00:00
Author
Owner

Done!

Done!
konrad marked this conversation as resolved
dpschen reviewed 2023-03-16 20:29:52 +00:00
src/composables/useDaytimeSalutation.ts Outdated
@ -11,2 +11,3 @@
const {t} = useI18n({useScope: 'global'})
const now = useNow()
const now = useNow({
interval: 30 * 60 * 1000, // Every 30 minutes
dpschen commented 2023-03-16 20:29:52 +00:00
Member

Same here.

Same here.
👍 1
konrad commented 2023-03-24 18:49:22 +00:00
Author
Owner

Done!

Done!
dpschen marked this conversation as resolved
WofWca commented 2023-03-17 10:55:23 +00:00
Contributor
Copy Link

@WofWca can you check if this fixes it?

Yes, it's better.

> @WofWca can you check if this fixes it? Yes, it's better.
konrad added 1 commit 2023-03-24 18:47:34 +00:00
konrad added 1 commit 2023-03-24 18:49:01 +00:00
dpschen reviewed 2023-03-24 18:57:36 +00:00
src/components/tasks/GanttChart.vue Outdated
@ -159,2 +158,3 @@
const today = useNow()
const today = ref(new Date())
onMounted(() => today.value = new Date())
dpschen commented 2023-03-24 18:57:36 +00:00
Member

I think it should be onActivated

I think it should be `onActivated`
dpschen commented 2023-03-24 18:58:29 +00:00
Member

Because it the view is cached mounted will not trigger again.

Because it the view is cached mounted will not trigger again.
konrad commented 2023-03-24 19:06:46 +00:00
Author
Owner

Ah yes, good point. Fixed.

Ah yes, good point. Fixed.
konrad marked this conversation as resolved
dpschen reviewed 2023-03-24 18:57:57 +00:00
src/composables/useDaytimeSalutation.ts Outdated
@ -11,2 +10,3 @@
const {t} = useI18n({useScope: 'global'})
const now = useNow()
const now = ref(new Date())
onMounted(() => now.value = new Date())
dpschen commented 2023-03-24 18:57:57 +00:00
Member

Same here

Same here
konrad commented 2023-03-24 19:07:51 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
konrad added 2 commits 2023-03-24 19:09:56 +00:00
continuous-integration/drone/pr Build is failing Details
b3ed637cec
fix: use time constant
konrad added 1 commit 2023-03-24 19:10:47 +00:00
continuous-integration/drone/pr Build is passing Details
17cf07140f
fix: update logo change only every hour
dpschen approved these changes 2023-03-24 23:30:12 +00:00
dpschen merged commit 71265769ce into main 2023-03-24 23:30:29 +00:00
dpschen deleted branch feature/add-interval-to-use-now 2023-03-24 23:30:29 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
dpschen
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
4 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#3255
No description provided.
Delete Branch "feature/add-interval-to-use-now"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?