feat: grid for list cards #2709

dpschen · 2022-11-15T16:33:45Z

dpschen commented

2022-11-15 16:33:45 +00:00

I had this branch in a wip state for a while already but rebased it now that I saw https://github.com/go-vikunja/frontend/pull/91/.

It takes inspiration from that branch but with some changes:

While I liked the idea of the colors only shown as a bar I think it was a bit too thick
the descrption isn't shown on the card. Reason beeing that there is simply not enough space if you have a longer list name. I did add the description on hover though.
I think the favorties button was broken. works now.

I had this branch in a wip state for a while already but rebased it now that I saw https://github.com/go-vikunja/frontend/pull/91/. It takes inspiration from that branch but with some changes: - While I liked the idea of the colors only shown as a bar I think it was a bit too thick - the descrption isn't shown on the card. Reason beeing that there is simply not enough space if you have a longer list name. I did add the description on hover though. - I think the favorties button was broken. works now.

dpschen added the

kind/feature

label 2022-11-15 16:33:45 +00:00

dpschen force-pushed feature/list-card-grid from 3cc3eff7f9 to 57f32b3649

2022-11-15 16:38:43 +00:00

Compare

frederick commented

2022-11-15 16:42:30 +00:00

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://2709-feature-list-card-grid--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://2709-feature-list-card-grid--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 changed title from ~~WIP: feat: grid for list cards~~ to feat: grid for list cards

2022-11-15 17:14:20 +00:00

konrad was assigned by dpschen

2022-11-15 17:14:26 +00:00

dpschen requested review from konrad 2022-11-15 17:14:28 +00:00

konrad reviewed 2022-11-16 15:04:20 +00:00

src/components/list/partials/ListCard.vue Outdated

						
				@ -0,0 +6,4 @@

							'has-background': blurHashUrl !== '' || background !== null

						}"

						:style="{

							'border-left': list.hexColor ? `0.25rem solid ${list.hexColor}` : undefined,

konrad commented

2022-11-16 14:58:33 +00:00

Can you only declare the color here and move the size and style in a stylesheet?

dpschen commented

2022-11-16 16:11:20 +00:00

yes! makes sense

dpschen commented

2022-11-17 11:40:31 +00:00

I thought about this again and realized that the reason I put all the border inline was that the border itself is meant to be optional.

In case we have no list.color we don't want a border, because if we would just make the border transparent in that case this offsets the padding but more important creates some distance for backgrounds.
I could have introduced a new class .has-border then set the width and style based on that class and additionally add the color via inline style or even set it as css variable. But that seemed more complex than the one line solution that I used now :)

I thought about this again and realized that the reason I put all the border inline was that the border itself is meant to be optional. In case we have no `list.color` we don't want a border, because if we would just make the border `transparent` in that case this offsets the padding but more important creates some distance for backgrounds. I could have introduced a new class `.has-border` then set the width and style based on that class and additionally add the color via inline style or even set it as css variable. But that seemed more complex than the one line solution that I used now :)

👍 1

konrad commented

2022-11-17 15:43:45 +00:00

I could have introduced a new class .has-border then set the width and style based on that class and additionally add the color via inline style or even set it as css variable. But that seemed more complex than the one line solution that I used now :)

That sounds reasonable.

> I could have introduced a new class .has-border then set the width and style based on that class and additionally add the color via inline style or even set it as css variable. But that seemed more complex than the one line solution that I used now :) That sounds reasonable.

konrad marked this conversation as resolved

src/components/list/partials/ListCard.vue Outdated

						
				@ -0,0 +62,4 @@

				<style lang="scss" scoped>

				.list-card {

					--list-card-padding: 1rem;

konrad commented

2022-11-16 15:00:00 +00:00

It looks like this variable is only used here. (I assume you wanted to use it as well in the .favorite declaration?

It looks like this variable is only used here. (I assume you wanted to use it as well in the `.favorite` declaration?

dpschen commented

2022-11-16 16:13:40 +00:00

I do??

top: var(--list-card-padding);
	right: var(--list-card-padding);

I do?? ``` top: var(--list-card-padding); right: var(--list-card-padding); ```

konrad commented

2022-11-16 16:19:56 +00:00

But the variable is only defined in the scope of .list-card and .favorites is not part of it. Or is it?

But the variable is only defined in the scope of `.list-card` and `.favorites` is not part of it. Or is it?

dpschen commented

2022-11-16 16:29:26 +00:00

list-card is the root class of the component (same name as component, only kebab case). That's a pattern I really love to use. CSS properties are super might this way. Especially when you set them via js :)

`list-card` is the root class of the component (same name as component, only kebab case). That's a pattern I really love to use. CSS properties are super might this way. Especially when you set them via js :)

dpschen commented

2022-11-17 11:44:22 +00:00

I did reuse the variable in the max-height of .list-title as well.

I did reuse the variable in the `max-height` of `.list-title` as well.

konrad commented

2022-11-17 15:42:30 +00:00

Ohhh that makes a lot of sense. Very nice!

konrad marked this conversation as resolved

src/components/list/partials/useListBackground.ts Outdated

						
				@ -0,0 +39,4 @@

								const backgroundPromise = listService.background(list.value).then((result) => {

									background.value = result

								})

								await Promise.all([blurHashPromise, backgroundPromise])

konrad commented

2022-11-16 15:03:26 +00:00

Don't the two promises need a return to actually resolve the promise? Or is that implicit?

Don't the two promises need a `return` to actually resolve the promise? Or is that implicit?

dpschen commented

2022-11-16 16:26:49 +00:00

Not sure if I got what you mean here, but trying to explan:

The handler of a watcher doesn't care if it has a return value or what it is.
By prefixing the handler with async the return value of the handler will automatically be a promise. If we don't return anything it will be a promise that resolves to undefined, which is fine for us, since as written above, the watch function doesnt care what value is returned by it's handler.

By not using await in front of getBlobFromBlurhash we get the promise as result, not the resolved value. Same true for for the background.

By using promise.all both promises do resolve now in parallel.
The await in front of the promise.all makes sure that the promise resolves before we set backgroundLoading to false again.

Not sure if I got what you mean here, but trying to explan: The handler of a watcher doesn't care if it has a return value or what it is. By prefixing the handler with `async` the return value of the handler will automatically be a promise. If we don't return anything it will be a promise that resolves to undefined, which is fine for us, since as written above, the watch function doesnt care what value is returned by it's handler. By not using await in front of getBlobFromBlurhash we get the promise as result, not the resolved value. Same true for for the background. By using promise.all both promises do resolve now in parallel. The await in front of the promise.all makes sure that the promise resolves before we set backgroundLoading to false again.

konrad commented

2022-11-17 15:42:01 +00:00

Makes sense (I've learnt something) but I thought of the .then. Does that simply resolve to undefined if no return value was specified? (but it does resolve without a return)?

Makes sense (I've learnt something) but I thought of the `.then`. Does that simply resolve to `undefined` if no return value was specified? (but it does resolve without a return)?

dpschen commented

2022-11-17 22:56:39 +00:00

I think that

function foo {
  return Promise.resolve()
}

and

async function foo {}

is the same

I think that ```ts function foo { return Promise.resolve() } ``` and ```ts async function foo {} ``` is the same

👍 1

konrad marked this conversation as resolved

dpschen force-pushed feature/list-card-grid from 57f32b3649 to c4044988dd

2022-11-17 11:43:29 +00:00

Compare

konrad approved these changes 2022-11-18 09:25:07 +00:00

konrad left a comment

Ready to merge once conflicts are resolved.

dpschen force-pushed feature/list-card-grid from c4044988dd to d4b961c0e1

2022-11-18 11:49:54 +00:00

Compare

dpschen force-pushed feature/list-card-grid from d4b961c0e1 to c3ff8b56b0

2022-11-18 11:51:37 +00:00

Compare

dpschen force-pushed feature/list-card-grid from c3ff8b56b0 to 42e9f306e8

2022-11-18 13:04:35 +00:00

Compare

dpschen scheduled this pull request to auto merge when all checks succeed 2022-11-18 13:04:45 +00:00

dpschen merged commit 42e9f306e8 into main

2022-11-18 13:15:01 +00:00

This repo is archived. You cannot comment on pull requests.

No reviewers

No Label

No Milestone

No project

No Assignees

3 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#2709