openfun / fun-richie-site-factory

🏭 Site factory to build France Université Numérique's web sites based on https://github.com/openfun/richie

GNU Affero General Public License v3.0

4 stars 2 forks source link

🍱(funmooc) Update 'funmooc' site integration to move up main menu and move down the search bar #224

Open sveetch opened 2 months ago

sveetch commented 2 months ago

Purpose of this PR is to change header structure so the main menu are moved up aside brand logo and search bar have been moved down aside breadcrumbs.

Since in the demo the main menu is too large for the available space, the signup button has been hided from CSS but frontend developer may prefer to do this at the richie-react login module level;
Base template disable the topbar_contact block content since funmooc was using it for the FAQ link which will be moved somewhere else further;
Breadcrumb part has been changed to include the search bar aside;
Sass changes on different components and objects have been made to perform the header and breadcrumbs adjustments;
Also some space adjustments have been made on header for>1024px,<xl breakpoints to ensure the main menu will fit;

Sample screenshots

Homepage, unlogged

Header without search bar (since it is still included in intro block)

homepage

Internal page

In various breakpoints

internal_page_medium

internal_page_large

internal_page_xl

Course list

This page has been left unchanged.

course_list

jbpenrath commented 1 month ago

After some discussion, on small breakpoint (your first screenshot, the search input should not be displayed next to the breadcrumb). Indeed, when the user opens the burger menu, it has access to the search input and next to the breadcrumb, the space is too small.

jbpenrath commented 1 month ago

There is a visual regression about the font weight and font family of menu entries. Previously they were bold and use Montserrat.

jbpenrath commented 1 month ago

There are some accessibilities contract issues with the hover state of the search input. IMO the placeholder font color should be darker and the magnify icon color should switch to a dark color.

jbpenrath commented 1 month ago

There is a layout issue between 990px and 1020px

jbpenrath commented 1 month ago

From 575px viewport width, a margin on the right of the search input is missing.

sveetch commented 1 month ago

@jbpenrath I have pushed two additional commits to fix issues you raised previously.

Proper font family and weight on menu are back;
Header logo has been shrinked from 194px to 178px, this was required to fix the many issues with horizontal space in various breakpoints;
In the same purpose, the menu font size decrease a little bit on breakpoint 992 to 1200px;
Search bar don't adopt a white background anymore when focused, alike the one in course list it does not change. This was required since we cannot change the 'magnifier' icon on focus since it depend from a class of your frontend component that is done only on the input, but the icon can not be included in the input DOM element so it can not change on focus;
Search bar is still in the breadcrumb container all along but it goes under the bread crumbs on small breakpoint. We cannot correctly push the searchbar in the mobile menu since it can not live in the menu HTML element, trying to move the search bar purely in CSS would be complicated and would take too many time;

I will squash the fix commits once we are done to merge.

Screenshot samples for breakpoints

news_576

news_768

news_992

news_1200

jbpenrath commented 1 week ago

@jbpenrath I have pushed two additional commits to fix issues you raised previously.

Proper font family and weight on menu are back;

Header logo has been shrinked from 194px to 178px, this was required to fix the many issues with horizontal space in various breakpoints;

In the same purpose, the menu font size decrease a little bit on breakpoint 992 to 1200px;

Search bar don't adopt a white background anymore when focused, alike the one in course list it does not change. This was required since we cannot change the 'magnifier' icon on focus since it depend from a class of your frontend component that is done only on the input, but the icon can not be included in the input DOM element so it can not change on focus;

Search bar is still in the breadcrumb container all along but it goes under the bread crumbs on small breakpoint. We cannot correctly push the searchbar in the mobile menu since it can not live in the menu HTML element, trying to move the search bar purely in CSS would be complicated and would take too many time;

I will squash the fix commits once we are done to merge.

Screenshot samples for breakpoints

Great!

Just on mobile, the search input is already displayed when needed so IMO, you just have to hide the search input from the breadcrumb on small viewport.

sveetch commented 1 week ago

@jbpenrath I have pushed two additional commits to fix issues you raised previously.

Great!

Just on mobile, the search input is already displayed when needed so IMO, you just have to hide the search input from the breadcrumb on small viewport.

It seemed that the new behavior felt better to some people from last visio. I propose to wait for them to check about if we keep it or switch to your proposal (restore the search form at its previous DOM position, show it on only on mobile, duplicate the search form at the new position, show it only on desktop).

sveetch commented 5 days ago

I've rebased my branch with last version and squashed commits for feedbacks