Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apps/budgeting: add anchor link and use in list #4782

Merged
merged 1 commit into from
Dec 29, 2022

Conversation

fuzzylogic2000
Copy link
Contributor

No description provided.

@fuzzylogic2000 fuzzylogic2000 marked this pull request as draft December 20, 2022 11:00
@fuzzylogic2000 fuzzylogic2000 force-pushed the kl-2022-12-proposal-anchor branch from 7db9708 to 3efe9df Compare December 21, 2022 09:26
@fuzzylogic2000 fuzzylogic2000 marked this pull request as ready for review December 21, 2022 09:31
@fuzzylogic2000 fuzzylogic2000 changed the title WIP: apps/budgeting: add anchor link and use in list apps/budgeting: add anchor link and use in list Dec 21, 2022
@philli-m philli-m self-assigned this Dec 21, 2022
Copy link
Contributor

@philli-m philli-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the page anchor should work looking at the code but it's not and I don't know why? tested on ff and chrome
also we can add scroll-behavior: smooth; to html to ensure it's not stuttery

@philli-m philli-m removed their assignment Dec 21, 2022
@fuzzylogic2000
Copy link
Contributor Author

the page anchor should work looking at the code but it's not and I don't know why? tested on ff and chrome also we can add scroll-behavior: smooth; to html to ensure it's not stuttery

@phillimorland It did work in Chromium. Will try again and add the smooth-thingy though!

@khamui khamui requested a review from philli-m December 22, 2022 11:15
Copy link
Contributor Author

@fuzzylogic2000 fuzzylogic2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khamui Nice! For me it only works when there are multiple proposals in the list?
Would be cool, if you could make one commit of the two.
And maybe @phillimorland can also review?

@philli-m philli-m self-assigned this Dec 28, 2022
Copy link
Contributor

@philli-m philli-m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep looks good, it works but the scroll to view almost scrolls too far so the top of the content is already going off screen, maybe we add a ref div about the list item?

@philli-m philli-m removed their assignment Dec 28, 2022
@khamui khamui force-pushed the kl-2022-12-proposal-anchor branch from 484e8dc to 80989eb Compare December 28, 2022 10:57
@khamui khamui requested a review from philli-m December 28, 2022 10:58
@khamui
Copy link
Contributor

khamui commented Dec 28, 2022

@phillimorland about the offset (top edge) we decided to go with a small offset for now. In case there needs to be a positioning in the center, this would be a story for later with a design.

@fuzzylogic2000 fuzzylogic2000 force-pushed the kl-2022-12-proposal-anchor branch from 80989eb to 4cd6536 Compare December 28, 2022 14:58
@fuzzylogic2000
Copy link
Contributor Author

Alright! Talked to @phillimorland and wil merge!

@fuzzylogic2000 fuzzylogic2000 merged commit 689391d into main Dec 29, 2022
@fuzzylogic2000 fuzzylogic2000 deleted the kl-2022-12-proposal-anchor branch December 29, 2022 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants