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

feat: restyle unauthenticated screens #1103

Merged
dpschen merged 29 commits from feature/login-pages into main 2021-12-12 16:40:14 +00:00
Conversation 12 Commits 29 Files Changed 10 +17 -1
1 changed files with 17 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 4abe1363ec - Show all commits

18
src/components/misc/no-auth-wrapper.vue
View File

@ -50,6 +50,10 @@ const motd = computed(() => store.state.config.motd)
border-radius: $radius;
box-shadow: var(--shadow-md);
overflow: hidden;
konrad marked this conversation as resolved Outdated
dpschen commented 2021-12-09 17:42:17 +00:00
Outdated
Review
Copy Link

Why overflow: hidden?

Why overflow: hidden?
konrad commented 2021-12-11 17:29:25 +00:00
Outdated
Review
Copy Link

To prevent the background from overflowing in the (rounded) corners.

To prevent the background from overflowing in the (rounded) corners.
@media screen and (max-width: $desktop) {
konrad marked this conversation as resolved Outdated
dpschen commented 2021-12-09 17:40:19 +00:00
Outdated
Review
Copy Link

Use mobile first:

	@media screen and (min-width: $desktop) {
    		border-radius: $radius;
    }

=> no need to reset the border-radius for mobile

Use mobile first: ```scss @media screen and (min-width: $desktop) { border-radius: $radius; } ``` => no need to reset the border-radius for mobile
border-radius: 0;
konrad marked this conversation as resolved Outdated
dpschen commented 2021-12-12 13:41:40 +00:00
Outdated
Review
Copy Link

Just checked this: The llama is so nice, it's too bad it's not visivle on mobile. How about adding a padding-bottom for mobile?

Just checked this: The llama is so nice, it's too bad it's not visivle on mobile. How about adding a padding-bottom for mobile?
konrad commented 2021-12-12 13:54:40 +00:00
Outdated
Review
Copy Link

done!

done!
🎉 1
}
}
.image {
@ -57,6 +61,14 @@ const motd = computed(() => store.state.config.motd)
background: url('@/assets/no-auth-image.jpg') no-repeat bottom;
konrad marked this conversation as resolved Outdated
dpschen commented 2021-11-28 15:43:08 +00:00
Outdated
Review
Copy Link

Use mobile first:

.image {
    @media screen and (max-width: $tablet) {
        display: none;
    }
    @media screen and (min-width: $tablet) {
        width: 40%;
        background: url('@/assets/no-auth-image.jpg') no-repeat bottom/cover;
        position: relative;
    }
    @media screen and (min-width: $desktop) {
        width: 60%;
    }
}

Compress image and load with something like https://github.com/JonasKruckenberg/imagetools

Use mobile first: ```scss .image { @media screen and (max-width: $tablet) { display: none; } @media screen and (min-width: $tablet) { width: 40%; background: url('@/assets/no-auth-image.jpg') no-repeat bottom/cover; position: relative; } @media screen and (min-width: $desktop) { width: 60%; } } ``` Compress image and load with something like https://github.com/JonasKruckenberg/imagetools
konrad commented 2021-12-11 19:44:06 +00:00
Outdated
Review
Copy Link

Compress image and load with something like https://github.com/JonasKruckenberg/imagetools

With the vite wrapper and use it for other images as well?

> Compress image and load with something like https://github.com/JonasKruckenberg/imagetools With the vite wrapper and use it for other images as well?
dpschen commented 2021-12-12 15:02:55 +00:00
Outdated
Review
Copy Link

Yes. But maybe let's move this to a new issue.

Yes. But maybe let's move this to a new issue.
👍 1
konrad commented 2021-12-12 15:03:23 +00:00
Outdated
Review
Copy Link

yeah I think we should.

yeah I think we should.
background-size: cover;
position: relative;
@media screen and (max-width: $desktop) {
width: 40%;
}
@media screen and (max-width: $tablet) {
display: none;
}
}
.overlay {
konrad marked this conversation as resolved
dpschen commented 2021-12-12 14:14:07 +00:00
Review
Copy Link

Move these rules to the @media screen and (min-width: $tablet) media query
=> makes it clearer that it just appies there.

	width: 50%;
	padding: 1rem;
	display: flex;
	flex-direction: column;
	justify-content: flex-end;
    
	&::after {
		content: '';
		position: absolute;
		top: 0;
		left: 0;
		right: 0;
		bottom: 0;
		background-color: rgba(0, 0, 0, .2);
		z-index: 10;
	}

	> * {
		z-index: 20;
	}

Move media queries before &::after and > *
=> those are styling other elements
=> move styles together that style the same stuff

If you change &::after in &::before you can remove the z-index if you replace
z-index: 20; with position: relative (this creates a new stacking context.
=> less complexity with managing z-index.

Move these rules to the `@media screen and (min-width: $tablet)` media query => makes it clearer that it just appies there. ```scss width: 50%; padding: 1rem; display: flex; flex-direction: column; justify-content: flex-end; &::after { content: ''; position: absolute; top: 0; left: 0; right: 0; bottom: 0; background-color: rgba(0, 0, 0, .2); z-index: 10; } > * { z-index: 20; } ``` Move media queries before `&::after` and `> *` => those are styling other elements => move styles together that style the same stuff If you change `&::after` in `&::before` you can remove the z-index if you replace `z-index: 20;` with `position: relative` (this creates a new [stacking context](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Positioning/Understanding_z_index/The_stacking_context). => less complexity with managing z-index.
konrad commented 2021-12-12 14:52:53 +00:00
Review
Copy Link

Done!

Done!
@ -82,12 +94,16 @@ const motd = computed(() => store.state.config.motd)
display: flex;
justify-content: space-between;
flex-direction: column;
@media screen and (max-width: $desktop) {
width: 100%;
}
}
.logo {
dpschen marked this conversation as resolved Outdated
dpschen commented 2021-12-09 18:17:56 +00:00
Outdated
Review
Copy Link

On desktop:
Maybe the logo should be not centered but left aligned with the content below.
=> Might make the Title easier to read because there is more room between those two headlines

On desktop: Maybe the logo should be not centered but left aligned with the content below. => Might make the Title easier to read because there is more room between those two headlines
konrad commented 2021-12-11 19:34:06 +00:00
Outdated
Review
Copy Link

You mean aligned in the middle like this?

You mean aligned in the middle like this?
konrad commented 2021-12-11 19:35:38 +00:00
Outdated
Review
Copy Link

Okay wow I can't put images in a comment here, here's the screenshot I wanted to post:

Okay wow I can't put images in a comment here, here's the screenshot I wanted to post: ![](https://kolaente.dev/attachments/1847041a-d987-46e1-9d70-99c5745e8306)
dpschen commented 2021-12-12 15:15:44 +00:00
Outdated
Review
Copy Link

I'm also not really happy with this. Let's keep it for now how it is.

I'm also not really happy with this. Let's keep it for now how it is.
color: var(--logo-text-color);
konrad marked this conversation as resolved Outdated
dpschen commented 2021-12-09 17:59:46 +00:00
Outdated
Review
Copy Link

Color seems to be applied twice (from inside the component aswell).

Color seems to be applied twice (from inside the component aswell).
max-width: 100%;
margin-bottom: 1rem;
margin: 1rem 0;
konrad marked this conversation as resolved Outdated
dpschen commented 2021-12-09 18:01:28 +00:00
Outdated
Review
Copy Link

Picky: new css love: use margin-block: 1rem; => transformed by postcss-preset-env. Reason: you don't want to define the inline (left, right) value, but do it here as a sideeffect.

Picky: new css love: use `margin-block: 1rem;` => transformed by postcss-preset-env. Reason: you don't want to define the inline (left, right) value, but do it here as a sideeffect.
konrad commented 2021-12-11 19:52:42 +00:00
Outdated
Review
Copy Link

hmm it looks like this isn't transformed. But it definitely is a nice propery.

hmm it looks like this isn't transformed. But it definitely is a nice propery.
dpschen commented 2021-12-12 14:45:31 +00:00
Outdated
Review
Copy Link

I checked this again.

I think that the browsers that we want to support according to our browserslist all support it natively so there is no need for postcss-preset-env to transform it =)

Because it does do that via postcss-logical

I checked this again. I think that the browsers that we want to support according to our browserslist all support it natively so there is no need for postcss-preset-env to transform it =) Because it does do that via [postcss-logical](https://github.com/csstools/postcss-plugins/tree/main/plugins/postcss-logical)
👍 1
}
.message {
konrad marked this conversation as resolved Outdated
dpschen commented 2021-12-09 17:38:38 +00:00
Outdated
Review
Copy Link

Add class to Message component and use that.
=> By using a class inside the component we would create an implicit dependency.

Add class to Message component and use that. => By using a class inside the component we would create an implicit dependency.
konrad commented 2021-12-11 19:37:57 +00:00
Outdated
Review
Copy Link

It looks like the class here wasn't used anywhere. I've removed it.

It looks like the class here wasn't used anywhere. I've removed it.
👍 1
dpschen commented 2021-12-12 14:21:19 +00:00
Outdated
Review
Copy Link

Use mobile first:

.content {
    // start with common stuff
    display: flex;
    justify-content: space-between;
    flex-direction: column;
	padding: 2rem 2rem 1.5rem;
    
    // define stuff that applies mobile only
	@media screen and (max-width: $desktop) {
        width: 100%;
		max-width: 450px;
		margin-inline: auto;
	}
    
    // define desktop only stuff
    @media screen and (min-width: $desktop) {
		width: 50%;
	}
}
Use mobile first: ```scss .content { // start with common stuff display: flex; justify-content: space-between; flex-direction: column; padding: 2rem 2rem 1.5rem; // define stuff that applies mobile only @media screen and (max-width: $desktop) { width: 100%; max-width: 450px; margin-inline: auto; } // define desktop only stuff @media screen and (min-width: $desktop) { width: 50%; } } ```
konrad commented 2021-12-12 14:56:14 +00:00
Outdated
Review
Copy Link

Done!

Done!