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

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

Merged
merged 9 commits into from
Feb 24, 2023

Conversation

romanhaa
Copy link

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 romanhaa requested a review from fedde-s August 18, 2022 12:23
@romanhaa
Copy link
Author

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
Copy link
Author

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
Copy link
Member

fedde-s commented Oct 19, 2022

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
Copy link
Author

romanhaa commented Oct 19, 2022

I vaguely remember that as well.

@romanhaa
Copy link
Author

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

@fedde-s
Copy link
Member

fedde-s commented Oct 19, 2022

@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 😄 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
Copy link
Member

fedde-s commented Oct 20, 2022

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 fedde-s changed the base branch from v0.1.0/docker to v0.2.5/docker October 20, 2022 09:26
@fedde-s
Copy link
Member

fedde-s commented Nov 4, 2022

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

@fedde-s
Copy link
Member

fedde-s commented Nov 4, 2022

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
Copy link
Author

romanhaa commented Nov 4, 2022

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 and others added 7 commits January 2, 2023 14:17
Keeping the Back to top button from being obscured.
The ProtVista protein structure viewer on the target profile page
rendered over the logo bar when scrolling past it, seemingly because the
logo bar existed in the DOM inside the local stacking context of another
app bar with a z-index value lower than 40001.

This still leaves the issue that the bar also overlays the 3d structure
viewer when it's expanded to fill the viewport, blocking some of the
buttons of the viewer.
@romanhaa romanhaa changed the base branch from v0.2.5/docker to v0.2.10/docker January 2, 2023 13:48
@romanhaa
Copy link
Author

romanhaa commented Jan 2, 2023

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
Copy link
Member

fedde-s commented Feb 14, 2023

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

@fedde-s
Copy link
Member

fedde-s commented Feb 20, 2023

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

@fedde-s fedde-s changed the base branch from v0.2.10/docker to v0.2.10/thehyve February 24, 2023 14:28
@fedde-s fedde-s merged commit 948a2b1 into v0.2.10/thehyve Feb 24, 2023
fedde-s added a commit that referenced this pull request Feb 24, 2023
louwenjjr pushed a commit that referenced this pull request Feb 28, 2023
alepev pushed a commit that referenced this pull request Apr 19, 2023
@fedde-s fedde-s mentioned this pull request Jun 5, 2023
louwenjjr pushed a commit that referenced this pull request Jun 13, 2023
louwenjjr pushed a commit that referenced this pull request Jun 14, 2023
fedde-s added a commit that referenced this pull request Jul 5, 2023
Co-authored-by: Fedde Schaeffer <[email protected]>
Co-authored-by: Alessia Peviani <[email protected]>
fedde-s added a commit that referenced this pull request Jul 5, 2023
Co-authored-by: Fedde Schaeffer <[email protected]>
Co-authored-by: Alessia Peviani <[email protected]>
@fedde-s fedde-s deleted the configurable-company-bar branch July 7, 2023 09:14
louwenjjr pushed a commit that referenced this pull request Jul 7, 2023
Co-authored-by: Fedde Schaeffer <[email protected]>
Co-authored-by: Alessia Peviani <[email protected]>
louwenjjr pushed a commit that referenced this pull request Jul 17, 2023
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.

2 participants