thehyve / ot-ui-apps

Apache License 2.0
1 stars 0 forks source link

Add configuration to show top bar with organisation logo and user info #1

Closed romanhaa closed 1 year ago

romanhaa commented 2 years ago

This PR allows showing a top bar with an organisation logo and optionally information about the currently logged in user. Merging this into our fork means we no longer have to commit this into project repositories.

romanhaa commented 2 years ago

A thought: The background and text color in the top bar are sometimes adjusted to fit with the org logo. We may want to allow configuration of that as well.

romanhaa commented 1 year ago

Implementing the top bar through a React component as opposed to injecting it directly into the frontend fixes two minor issues: (1) the arrow at the bottom of the home page (see below) is cut off because the page content is pushed down by the top bar and (2) the scrolling issue (when scrolling down from the top of the page, there is a small jump).

Screenshot 2022-10-19 at 10 47 05

fedde-s commented 1 year ago

The previous approach, while working around the awkwardness of patching things that should have been configurable into the upstream code before building, caused a third layout issue where the bar would obscure widgets with absolute or fixed positioning at the very top of the page, such as the back-to-top button in the navbar of OT Platform profile pages and the button to reset the expanded view of a molecular 3d structure.

romanhaa commented 1 year ago

I vaguely remember that as well.

romanhaa commented 1 year ago

Maybe we can already implement it as a shared component between OTP and OTG, I think that would be ideal.

fedde-s commented 1 year ago

@romanhaa:

Maybe we can already implement it as a shared component between OTP and OTG

I already assumed that that would be the next action after merging this one, if not the next commit before doing so :smile: I imagine that releases of OTP and OTG usually use different snapshots of the ot-ui-apps code so we don't deploy the same commit (this PR still points to the one that was used in the previous release of the platform), but maintaining different sets of patches between the two apps sounds needlessly complicated.

fedde-s commented 1 year ago

So far, (rebased on 0.2.5/docker for platform 22.09) I've still found two issues of the same scale as the ones it is supposed to fix: the back-to-top button in the profile page navbar is still obscured by the top bar and the 3d structure view (in the ProtVista section on target profile pages) now moves in front of the top bar even when just scrolling by it (rather than expanding to fill the window): Screenshot showing the top bar obscuring the Home button in a scrolled-up navbar and not obscuring the 3d structure view

I'll start looking whether I can fix them.

fedde-s commented 1 year ago

I've addressed the issues I found. @romanhaa, would you have time to review the four added commits at some point?

fedde-s commented 1 year ago

The last commit is a refactoring of the one before; to avoid rebase conflicts in the future, we should squash at least those two.

romanhaa commented 1 year ago

Very nice, thanks for polishing this and taking care of all the (discovered) bugs. The last thing that comes to my mind is that it would be great if the top bar could also be enabled for OTG. When I started this PR, OTG wasn't using this repo for the frontend yet but now we could really make use of shared components.

romanhaa commented 1 year ago

I rebased the commits on the v0.2.10/docker branch, changed the base branch accordingly, moved the top bar definition to the shared ui package and implemented the top bar in OTG.

fedde-s commented 1 year ago

As discussed, I added logic to call getLoggedInUser() if it is defined.

fedde-s commented 1 year ago

Still working on the final test in OTG before squash-merging this in preparation for platform 23.02