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

Implement conditional navbar for pages #694

Merged
merged 21 commits into from
Nov 21, 2023

Conversation

NPDebs
Copy link
Collaborator

@NPDebs NPDebs commented Oct 30, 2023

This PR addresses issue #649.
With the newly created /openseeds subsite, there is a need to display a navbar with a different look and content.
Thus, the changes made in this PR were:

  1. create a new navbar (openseeds-navbar.html),
  2. add a statement to the index.html layout that displays a navbar depending on which url is requested.

we-are-ols)

openseeds

Note for reviewer(s):
The both navbars look the same for now because we are in the process of:

  1. deciding what information stays in each navbar.
  2. getting the theme (which contains the two different logos) to work.

Thanks for taking the time to review!

Copy link
Member

@bebatut bebatut left a comment

Choose a reason for hiding this comment

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

Thanks @NPDebs
I put some quick comments there
It might be good to modify also the default header to change the navbar

_includes/openseeds-navbar.html Outdated Show resolved Hide resolved
_includes/openseeds-navbar.html Outdated Show resolved Hide resolved
_includes/openseeds-navbar.html Outdated Show resolved Hide resolved
_includes/openseeds-navbar.html Outdated Show resolved Hide resolved
@NPDebs NPDebs marked this pull request as ready for review October 31, 2023 08:49
_includes/openseeds-header.html Outdated Show resolved Hide resolved
_includes/openseeds-header.html Show resolved Hide resolved
<a class="navbar-item" href="{% link about.md %}#mentors"> Mentors </a>
<a class="navbar-item" href="{% link about.md %}#experts"> Experts </a>
<a class="navbar-item" href="{% link about.md %}#facilitators"> Facilitators </a>
<a class="navbar-item navbar-previous-title" href="{% link openseeds/index.md %}#cohorts"> Cohorts </a>
Copy link
Member

Choose a reason for hiding this comment

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

This could be maybe moved as a dedicated dropdown menu, linking then to the different cohorts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hope I understood this and executed it correctly. I will leave the comment unresolved, just in case 😄

_includes/openseeds-header.html Outdated Show resolved Hide resolved
@@ -45,7 +45,7 @@
</div>
Copy link
Member

Choose a reason for hiding this comment

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

It might be good to modify the navbar above.

OLS-8 should be maybe removed. We could also then have a menu for each of the 3 pillars

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Definitely! I have just added a new navbar dropdown that contains our 3 pillars.
Since this captures the consulting service as well (under open incubator), I think we can remove the Services navbar-link. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Screenshot (592)

Copy link
Member

@bebatut bebatut left a comment

Choose a reason for hiding this comment

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

I think each pillar should be a dropdown in the header

_includes/default-header.html Outdated Show resolved Hide resolved
_includes/default-header.html Outdated Show resolved Hide resolved
_includes/default-header.html Outdated Show resolved Hide resolved
_includes/default-header.html Outdated Show resolved Hide resolved
_includes/default-header.html Outdated Show resolved Hide resolved
_includes/default-header.html Outdated Show resolved Hide resolved
_includes/default-header.html Outdated Show resolved Hide resolved
<a class="navbar-item" href="#">WT impact research</a>
<a class="navbar-item" href="#">Turing Skills Policy Award</a>
<a class="navbar-item navbar-previous-title" href="#">Open Incubator</a>
<a class="navbar-item" href="#">Resident fellows program</a>
Copy link
Member

Choose a reason for hiding this comment

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

There is no section in the open-incubator.md file to link to that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

About that...

You're right. The individual pages for the 3 Pillars still need more content. You may have noticed in PR #688 that only open-research.md seems fully developed. The other two? Not so much.

I think Yo is in the process of writing the content to be added.

Until we can get more information added to open-incubator.md, we probably have two options:

  1. Merge this PR as is (considering all that is left is what Yo will give us).
  2. Wait for PR 688 to be complete, and link appropriately before merging this.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

P.S. The tests are failing because we don't have the open.research.md, etc mentioned in the navbar. It should be fine once the other PR is ready (or we can remove the links and use # for now).

Basically, no cause for alarm. 😄

Copy link
Member

Choose a reason for hiding this comment

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

I would wait to merge this PR than the other one is merged so everything could be fixed and in place

@NPDebs
Copy link
Collaborator Author

NPDebs commented Nov 14, 2023

Thanks a lot for the suggestions you made. The PR definitely looks richer! 💯🌟

@bebatut bebatut merged commit b321bc5 into open-life-science:main Nov 21, 2023
6 of 9 checks passed
@bebatut
Copy link
Member

bebatut commented Nov 21, 2023

Thanks a lot @NPDebs !!!!

@NPDebs NPDebs deleted the conditional_nav branch July 24, 2024 13:35
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