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

feat: header improvements #3007

Merged
dpschen merged 1 commits from dpschen/frontend:feature/header-improvements into main 2023-02-09 14:29:24 +00:00
Conversation 6 Commits 1 Files Changed 10 +194 -227
dpschen commented 2023-01-24 21:09:27 +00:00
Member
Copy Link

Based on feature/feat-sw-and-header-improvements

Based on `feature/feat-sw-and-header-improvements`
dpschen requested review from konrad 2023-01-24 21:09:32 +00:00
dpschen added the
kind/feature
 label 2023-01-24 21:09:38 +00:00
konrad was assigned by dpschen 2023-01-24 21:09:43 +00:00
konrad reviewed 2023-01-24 21:45:05 +00:00
src/components/home/TheNavigation.vue Outdated
@ -26,0 +14,4 @@
v-if="currentList.id"
class="list-title-wrapper"
>
<div class="list-title">{{ currentList.title === '' ? $t('misc.loading') : getListTitle(currentList) }}</div>
konrad commented 2023-01-24 21:45:05 +00:00
Owner

Isn't <h1> supposed to only contain text?

Isn't `<h1>` supposed to only contain text?
dpschen commented 2023-01-24 21:59:28 +00:00
Author
Member

I never heard that. Where do you take that info from? My idea here was that if a user jumps the page structure he'll automatically find headline text and the related buttons together. I could also switch the div (wrapping the txt) and the h1.

I never heard that. Where do you take that info from? My idea here was that if a user jumps the page structure he'll automatically find headline text and the related buttons together. I could also switch the `div` (wrapping the txt) and the `h1`.
konrad commented 2023-01-24 22:01:09 +00:00
Owner

Not sure where exactly that comes from, might as well be wrong. It just "feels" wrong to put a lot of other stuff in a headline element.

I think semantically it'd be cleaner to switch the div and h1.

Not sure where exactly that comes from, might as well be wrong. It just "feels" wrong to put a lot of other stuff in a headline element. I think semantically it'd be cleaner to switch the `div` and `h1`.
konrad commented 2023-01-24 22:04:02 +00:00
Owner

I was able to find this: https://www.quora.com/Is-the-usage-of-DIV-within-an-H1-tag-bad-for-SEO-If-yes-why

But it's about SEO which is not relevant here.

CSS-Tricks has another little one here: https://css-tricks.com/can-include-a-certain-html-element-within-another-certain-html-element/

I was able to find this: https://www.quora.com/Is-the-usage-of-DIV-within-an-H1-tag-bad-for-SEO-If-yes-why But it's about SEO which is not relevant here. CSS-Tricks has another little one here: https://css-tricks.com/can-include-a-certain-html-element-within-another-certain-html-element/
dpschen commented 2023-01-25 13:29:08 +00:00
Author
Member

Yeah, fuck SEO :D

I replaced it because of semantics. That I agree more with.
In reality I guess no browsers nowadays really has an issue with the h1 being a wrapper.

The caniinclude site also doesn't seem to be up-to-date.
E.g. https://caninclude.glitch.me/caninclude?child=h1&parent=a
While not without problems, it got really common to wrap stuff with a link for example for block links.
AFAIK most browsers support this.

Yeah, fuck SEO :D I replaced it because of semantics. That I agree more with. In reality I guess no browsers nowadays really has an issue with the `h1` being a wrapper. The caniinclude site also doesn't seem to be up-to-date. E.g. https://caninclude.glitch.me/caninclude?child=h1&parent=a While not without problems, it got really common to wrap stuff with a link for example [for block links](https://css-tricks.com/a-complete-guide-to-links-and-buttons/#aa-links-around-bigger-chunks-of-content). AFAIK most browsers support this.
👍 1
dpschen marked this conversation as resolved
frederick commented 2023-01-24 21:50:46 +00:00
Member
Copy Link

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3007-feature-header-improvements--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://3007-feature-header-improvements--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 feature/header-improvements from 01e52b956c to 5ebf77fce1 2023-01-25 13:17:39 +00:00 Compare
konrad requested changes 2023-01-25 14:20:08 +00:00
konrad left a comment
Owner

Looks like the list title is not centered anymore:

image

Looks like the list title is not centered anymore: ![image](/attachments/d4dfacca-13b7-490c-b2d8-cb85f6d71568)
image.png
28 KiB
👍 1 ✅ 1
dpschen force-pushed feature/header-improvements from 5ebf77fce1 to 6508ce1ccd 2023-01-25 15:24:34 +00:00 Compare
dpschen commented 2023-01-25 15:24:50 +00:00
Author
Member
Copy Link

Looks like the list title is not centered anymore:

image

Fixed

> Looks like the list title is not centered anymore: > > ![image](/attachments/d4dfacca-13b7-490c-b2d8-cb85f6d71568) Fixed
dpschen requested review from konrad 2023-01-25 15:24:55 +00:00
konrad approved these changes 2023-01-25 15:30:34 +00:00
konrad left a comment
Owner

Looks good

Looks good
konrad scheduled this pull request to auto merge when all checks succeed 2023-01-25 15:30:42 +00:00
konrad canceled auto merging this pull request when all checks succeed 2023-01-26 11:01:02 +00:00
konrad commented 2023-01-26 11:01:17 +00:00
Owner
Copy Link

Looks like the tests need adjustments as their selectors changed.

Looks like the tests need adjustments as their selectors changed.
👍 1
dpschen force-pushed feature/header-improvements from 6508ce1ccd to eb4d8aad6b 2023-02-08 15:39:35 +00:00 Compare
dpschen commented 2023-02-08 15:54:07 +00:00
Author
Member
Copy Link

I don't understand why this doesn't run through. For me the tests succeed locally.
Do you have an idea?

I don't understand why this doesn't run through. For me the tests succeed locally. Do you have an idea?
konrad commented 2023-02-08 17:36:40 +00:00
Owner
Copy Link

I don't understand why this doesn't run through. For me the tests succeed locally.
Do you have an idea?

The test looks for a .list-title h1 but the element is now a h1.list-title.

> I don't understand why this doesn't run through. For me the tests succeed locally. > Do you have an idea? The test looks for a `.list-title h1` but the element is now a `h1.list-title`.
dpschen force-pushed feature/header-improvements from eb4d8aad6b to 19709978b3 2023-02-09 14:04:15 +00:00 Compare
dpschen force-pushed feature/header-improvements from 19709978b3 to e8db2c2b45 2023-02-09 14:19:48 +00:00 Compare
dpschen commented 2023-02-09 14:28:47 +00:00
Author
Member
Copy Link

I don't understand why this doesn't run through. For me the tests succeed locally.
Do you have an idea?

The test looks for a .list-title h1 but the element is now a h1.list-title.

Okay, was some local caching issue with cypress…

> > I don't understand why this doesn't run through. For me the tests succeed locally. > > Do you have an idea? > > The test looks for a `.list-title h1` but the element is now a `h1.list-title`. Okay, was some local caching issue with cypress…
dpschen merged commit e8db2c2b45 into main 2023-02-09 14:29:24 +00:00
dpschen deleted branch feature/header-improvements 2023-02-09 14:29:24 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
konrad
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
konrad
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#3007
No description provided.
Delete Branch "dpschen/frontend:feature/header-improvements"

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?