Skip to content

Dashboard tab marker is now set via pat-navigation.#447

Open
thet wants to merge 1 commit intomainfrom
pat-navigation--dashboard-tab
Open

Dashboard tab marker is now set via pat-navigation.#447
thet wants to merge 1 commit intomainfrom
pat-navigation--dashboard-tab

Conversation

@thet
Copy link
Copy Markdown
Member

@thet thet commented Sep 19, 2022

Removed the backend code which sets the current class in the dashboard.
Instead it is set via pat-navigation client side only.

Merge together:

#447
https://gitlab.com/syslabcom/daimler/daimler.oira/-/merge_requests/144

Related:

@thet thet marked this pull request as draft September 19, 2022 12:47
@thet thet requested review from ale-rt and reinhardt September 19, 2022 12:47
@ale-rt
Copy link
Copy Markdown
Member

ale-rt commented Sep 19, 2022

Removed the backend code which sets the current class in the dashboard. Instead it is set via pat-navigation client side only.

BEFORE‌ MERGING, please see: https://gitlab.com/syslabcom/daimler/daimler.oira/-/merge_requests/144

From your analysis on that PR we have some cases that the previous code covered and this one did not

@reinhardt
Copy link
Copy Markdown
Contributor

On the one hand I think the behaviour with the dashboard switcher in daimler.oira is not ideal, on the other hand it seems to point to a general problem: In the backend we can easily distinguish between context and view; the javascript has to work on the URL where it isn't clear which names are context IDs and which are view names.

AFAICT merging this would not make the behaviour worse in plain Euphorie. It would make the code simpler (but only a little). We could still keep the old code for daimler.oira. However, that would introduce yet another deviation between the two.

If we don't merge this PR, will there be any conflict with the new pat-navigation javascript?

@thet
Copy link
Copy Markdown
Member Author

thet commented Sep 20, 2022

@reinhardt we can just not merge this, that should be fine.

@thet thet force-pushed the pat-navigation--dashboard-tab branch from 4bcb141 to 49bd341 Compare September 27, 2022 13:41
@thet
Copy link
Copy Markdown
Member Author

thet commented Sep 27, 2022

@ale-rt I‌ re-checked this PR with the current pat-navigation changes and it does work fine for me. I think we can merge this.

@thet thet marked this pull request as ready for review September 27, 2022 13:50
@ale-rt
Copy link
Copy Markdown
Member

ale-rt commented Sep 27, 2022

I am not sure I am testing the right code, but before the patch after the login the dashboard was highlighted now it is not:
image
It seems to me that there is an issue handling the fragment.

BTW it is better if we have no tab highlighted rather than two.

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