enh(App): navigation tweaks for Contexts #1080
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
juliusknorr
reviewed
May 8, 2024
juliusknorr
reviewed
May 8, 2024
| const contextId = loadState('tables', 'contextId', undefined); | ||
| const originalUrl = window.location.href | ||
| this.$router.replace('/application/' + contextId).catch(() => {}) | ||
| // reverts turning /apps/tables/app/28 into /apps/tables/app/28#/application/28 |
There was a problem hiding this comment.
Seems like an OK approach for now.
We could also change the vue-router so that it uses the exact same routes as the backend defines, but that might require a few more changes to have the old urls still work.
nextcloud/deck@9f98f0b
https://github.com/nextcloud/deck/pull/1977/files
There was a problem hiding this comment.
interesting switch! Thanks for the pointer
juliusknorr
approved these changes
May 8, 2024
blizzz
force-pushed
the
enh/933/navigation
branch
from
2ff034d
to
faa194e
Compare
- new page endpoint for contexts - no Tables navigation in "standalone" contexts Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
blizzz
force-pushed
the
enh/933/navigation
branch
from
faa194e
to
c0cc6fd
Compare
|
Do we want this also in 0.7? |
|
/backport to stable0.7 |
2 tasks
backportbot
bot
removed
the
backport-request
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a more finegrained approach to to #933. The PR introduces an endpoint for each Context, so they can have the correct navigation item highlighted from the beginning, solving a known papercut from #1037
It also sets an initial state, so that in Vue we can apply the actual route. Alas, the Vue Router adds the same information into the address bar, duplicating "app[lication]" and the context id. This is not nice. I reset it to the original URL (accepting #/), but it comes at the cost of flickering. In Vue the route cannot be applied without changing the address bar, which is a known drawback.
Another tweak: I disable the app navigation, when the Context is opened from its specific URL, making it more "standalone". Questionable if we want it. There are alternatives also: filtering the Tables and Views to only those included in the Context, and not showing applications; or (perhaps in future) a Context-specific app navigation (maybe relevant once we make use of pages).
What is not included in the Controller is a check whether a Context is actually having a navigation bar entry. So far we did not expose the setting, so all Contexts are shown there anyway. But even if not, I am not sure this is an issue (user has access anyways).
Please check the annotated video for a recorded quick tour.
context-nav-tweaks.mp4