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

Feature/local navigation #2501

Merged
merged 33 commits into from
Oct 24, 2022
Merged

Feature/local navigation #2501

merged 33 commits into from
Oct 24, 2022

Conversation

RasmusTraeholt
Copy link
Collaborator

@RasmusTraeholt RasmusTraeholt commented Sep 21, 2022

Which issue does this PR close?

This PR closes #2186

What is the new behavior?

Added two new experimental features.

  1. A new kirby-page directive [kirbyPageStickyContent] which allows custom content to be inserted between the kirby-page-title and the kirby-page-content. That will become fixed beneath the ion-header, when scrolling to its intersection point.
  2. A new local-navigation component <kirby-page-local-navigation> that can be used in conjunction with the new kirby-page directive.

Other:

  • A new KirbyExperimentalModule in the designsystem has been introduced in which experimental components can be declared and exposed.
  • A new ExperimentalExamplesModule along with a new route /examples/experimental/[path-to-experiment] has been introduced in the cookbook app in which experimental features can be created and viewed (the new route does not have a ui link)

Does this PR introduce a breaking change?

  • Yes
  • No

Are there any additional context?

Checklist:

The following tasks should be carried out in sequence in order to follow the process of contributing correctly.

Reminders

  • Make sure you have implemented tests following the guidelines in: "The good: Test".
  • Make sure you have updated the cookbook with examples and showcases (for bug fixes, enhancements & new components).

Review

  • Do a self-review.
  • Request that the changes are code-reviewed
  • Request that the changes are UX reviewed (only necessary if your PR introduces visual changes)

When the pull request has been approved it will be merged to develop by Team Kirby.

@github-actions github-actions bot temporarily deployed to pr-local-navigation September 21, 2022 14:22 Inactive
@github-actions github-actions bot temporarily deployed to pr-local-navigation September 22, 2022 07:00 Inactive
@RasmusTraeholt RasmusTraeholt force-pushed the feature/local-navigation branch from 3083473 to d686cb6 Compare September 22, 2022 08:32
@github-actions github-actions bot temporarily deployed to pr-local-navigation September 22, 2022 08:32 Inactive
@github-actions github-actions bot temporarily deployed to pr-local-navigation September 22, 2022 08:38 Inactive
@RasmusTraeholt RasmusTraeholt marked this pull request as ready for review September 22, 2022 08:47
@RasmusTraeholt RasmusTraeholt requested review from RasmusKjeldgaard and removed request for jkaltoft September 22, 2022 14:14
@RasmusKjeldgaard RasmusKjeldgaard requested review from mictro and removed request for RasmusKjeldgaard September 23, 2022 09:41
@github-actions github-actions bot temporarily deployed to pr-local-navigation October 3, 2022 09:09 Inactive
@github-actions github-actions bot temporarily deployed to pr-local-navigation October 3, 2022 14:02 Inactive
@jkaltoft
Copy link
Collaborator

I'm on vacation next week so I've approved the changes in advance 🙂

If you have any questions or comments we can follow up on it the week after next week.

@RasmusTraeholt
Copy link
Collaborator Author

I'm guessing this solution was chosen because we need the border to exceed the width of .container on narrow viewports, but I'm not completely sure. By looking at the code itself it's not immediately obvious why it can't be done with just a 1-line border-bottom declaration on .container.

Theres a bit more too it than just breaking constraints. The selected bottom border itself needs to have rounded corners (on all four), which is also part of the reason that it is not "just" a bottom border.

@RasmusTraeholt
Copy link
Collaborator Author

* When using a solution based more or less entirely on a blog post or similar add a reference to the source(s). They can provide context and explain the solutions and the problems they solve in more detail than what fits in a comment 🙂

What solutions are you referring too?

For example, https://css-tricks.com/bold-on-hover-without-the-layout-shift/ has a good description of the same technique that you're using to avoid layout shifts when using bold text as hover effect.

Nice find. I won't reference something i did not see or base my solution on though. But i can extend the explanation a bit :)
I believe i saw a comment on stackoverflow with a suggestion to insert a pseudo-element to avoid layout shifts. But the question it was posted to had very little in common with what i wanted to achieve. So i wont reference that either, since it could cause more confusion than relevant context.

@github-actions github-actions bot temporarily deployed to pr-local-navigation October 17, 2022 11:46 Inactive
@github-actions github-actions bot temporarily deployed to pr-local-navigation October 17, 2022 12:09 Inactive
@github-actions github-actions bot temporarily deployed to pr-local-navigation October 17, 2022 13:22 Inactive
@github-actions github-actions bot temporarily deployed to pr-local-navigation October 18, 2022 06:53 Inactive
@mictro mictro requested a review from henrikvoetmand October 20, 2022 07:47
Copy link
Collaborator

@jkaltoft jkaltoft left a comment

Choose a reason for hiding this comment

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

Good work, Rasmus 👍🏻

The only thing left is to undo bumping the version number.

package.json Outdated Show resolved Hide resolved
From code owner suggestion

Co-authored-by: Jesper Kaltoft Vind <git@jesperkaltoft.dk>
@github-actions github-actions bot temporarily deployed to pr-local-navigation October 24, 2022 08:37 Inactive
@henrikvoetmand
Copy link
Collaborator

UX/UI review is OK!
There might be adjustments when we align with Toolbar component.

@mictro
Copy link
Contributor

mictro commented Oct 24, 2022

It seems to look good to all of us....
download

@RasmusTraeholt RasmusTraeholt added the merge:ready Will cause the pull-request to automatically be merged once it passes all checks. label Oct 24, 2022
@mictro mictro enabled auto-merge October 24, 2022 11:25
@mictro mictro merged commit 61c50e5 into develop Oct 24, 2022
@mictro mictro deleted the feature/local-navigation branch October 24, 2022 11:29
@RasmusKjeldgaard RasmusKjeldgaard mentioned this pull request Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge:ready Will cause the pull-request to automatically be merged once it passes all checks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[COMPONENT] Local navigation bar
4 participants