feat: settings background script setup #2104

Merged
konrad merged 1 commits from dpschen/frontend:feature/feat-settings-background-script-setup into main 1 month ago
Collaborator

This is already a bit older. I did the conversion without testing. So probably there are some bugs.

This is already a bit older. I did the conversion without testing. So probably there are some bugs.
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://2104-feature-feat-settings-background--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://2104-feature-feat-settings-background--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.
Owner

Can you investigate the CI failure? It seems unrelated to the changes you made in the PR but it only happens here.

Can you investigate the CI failure? It seems unrelated to the changes you made in the PR but it only happens here.
Owner

Looks like this was caused by recent changes in the api, fixed in a073cfac66 - can you rebase on main?

Looks like this was caused by recent changes in the api, fixed in https://kolaente.dev/vikunja/frontend/commit/a073cfac66b7b82b1b137f02385402f9c2d6cfdd - can you rebase on main?
dpschen force-pushed feature/feat-settings-background-script-setup from 1edd1f0a92 to 4c1422e635 3 months ago
Owner

Looks like the styling for unsplash background seelction is now broken:

image

And there's a "create" button on the bottom right corner:

image

(not sure if that hasn't been there before)

Other than that it seems to work fine.

Looks like the styling for unsplash background seelction is now broken: ![image](/attachments/c1ee822f-d2aa-4933-9c6f-939f3e4193f7) And there's a "create" button on the bottom right corner: ![image](/attachments/7bcdb22e-360a-4186-b78e-df6b5e9498e5) (not sure if that hasn't been there before) Other than that it seems to work fine.
Owner

Okay looks like these issues already exist on the main branch, interesting.

Would you mind taking a look into fixing these? I think it makes sense to merge this PR first or base the changes upon this one.

Okay looks like these issues already exist on the main branch, interesting. Would you mind taking a look into fixing these? I think it makes sense to merge this PR first or base the changes upon this one.
Poster
Collaborator

Okay looks like these issues already exist on the main branch, interesting.

Would you mind taking a look into fixing these? I think it makes sense to merge this PR first or base the changes upon this one.

Will investigate! 👍

> Okay looks like these issues already exist on the main branch, interesting. > Would you mind taking a look into fixing these? I think it makes sense to merge this PR first or base the changes upon this one. Will investigate! 👍
dpschen force-pushed feature/feat-settings-background-script-setup from 4c1422e635 to e17faaa7b2 3 months ago
Poster
Collaborator

The general issues with styling in the modal should be solved by #2109.

This branch now is based on that fix.

I'm not sure about how the buttons should be though.
Can you explain what is wrong here / should be changed or fixed?

The general issues with styling in the modal should be solved by #2109. This branch now is based on that fix. I'm not sure about how the buttons should be though. Can you explain what is wrong here / should be changed or fixed?
dpschen changed title from WIP: feat: settings background script setup to feat: settings background script setup 3 months ago
dpschen requested review from konrad 3 months ago
konrad was assigned by dpschen 3 months ago
dpschen added the
kind/feature
area/internal-code
labels 3 months ago
konrad reviewed 3 months ago
<x-button
:loading="backgroundUploadService.loading"
@click="$refs.backgroundUploadInput.click()"
@click="backgroundUploadInput?.click()"
Owner

Does this generate an error message when backgroundUploadInput is undefined or null? Or is vue smart enough to figure this out and prevent an error?

Does this generate an error message when `backgroundUploadInput` is undefined or null? Or is vue smart enough to figure this out and prevent an error?
Poster
Collaborator

I think this compiles to @click="null"

I think this compiles to `@click="null"`
dpschen marked this conversation as resolved
Owner

The general issues with styling in the modal should be solved by #2109.

Thanks!

I'm not sure about how the buttons should be though.
Can you explain what is wrong here / should be changed or fixed?

Mostly it's the create button which does not make sense here IMHO. I think we can get rid of it completely.

> The general issues with styling in the modal should be solved by #2109. Thanks! > I'm not sure about how the buttons should be though. Can you explain what is wrong here / should be changed or fixed? Mostly it's the create button which does not make sense here IMHO. I think we can get rid of it completely.
Poster
Collaborator

Mostly it's the create button which does not make sense here IMHO. I think we can get rid of it completely.

@konrad
The button is provied by the CreateEdit component.
How would you adjust that in order to optionally show that button?

> Mostly it's the create button which does not make sense here IMHO. I think we can get rid of it completely. @konrad The button is provied by the CreateEdit component. How would you adjust that in order to optionally show that button?
Owner

@dpschen Even though a bit ugly the best way is probably to add another prop to the component.

@dpschen Even though a bit ugly the best way is probably to add another prop to the component.
Poster
Collaborator

@dpschen Even though a bit ugly the best way is probably to add another prop to the component.

I always thought that you could check the attrs if onPrimary is set. I tested that and it was never set. 🤷‍♂️

> @dpschen Even though a bit ugly the best way is probably to add another prop to the component. I always thought that you could check the attrs if `onPrimary` is set. I tested that and it was never set. 🤷‍♂️
Owner

Isn't onPrimary an event handler? (didn't look at the code, this is from memory)

I thought of adding another property showPrimaryActionButton even though that's a bit ugly.

Isn't `onPrimary` an event handler? (didn't look at the code, this is from memory) I thought of adding another property `showPrimaryActionButton` even though that's a bit ugly.
Poster
Collaborator

Isn't onPrimary an event handler? (didn't look at the code, this is from memory)

I thought of adding another property showPrimaryActionButton even though that's a bit ugly.

It is. But based on this explanation the attrs object should contain onPrimary.

> Isn't `onPrimary` an event handler? (didn't look at the code, this is from memory) > > I thought of adding another property `showPrimaryActionButton` even though that's a bit ugly. It is. But based on [this explanation](https://v3-migration.vuejs.org/breaking-changes/listeners-removed.html#_3-x-syntax) the attrs object should contain `onPrimary`.
dpschen force-pushed feature/feat-settings-background-script-setup from e17faaa7b2 to 5e3eece3cc 2 months ago
dpschen force-pushed feature/feat-settings-background-script-setup from 5e3eece3cc to 84168a98e4 2 months ago
dpschen force-pushed feature/feat-settings-background-script-setup from 84168a98e4 to 0dcb433196 2 months ago
konrad approved these changes 1 month ago
konrad left a comment
Owner

Thanks!

Thanks!
konrad merged commit ff655808b3 into main 1 month ago
konrad deleted branch feature/feat-settings-background-script-setup 1 month ago

Reviewers

konrad approved these changes 1 month ago
continuous-integration/drone/pr Build is passing
The pull request has been merged as ff655808b3.
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#2104
Loading…
There is no content yet.