Problem/Motivation

The existing blocks available on the Navigation have the option to have a Label/Title/Heading but currently it isn't styled.

Steps to reproduce

  • Log in
  • Navigate to /admin/config/user-interface/navigation-block
  • Add a new block by clicking on the button "Place navigation block" or Configure an existing block.
  • On the form of the block, mark the checkbox "Display title" and save
  • Verify the new or edited block has a title on the admin navigation

Proposed resolution

Apply the existing designs for this label so it takes the right amount of attention while serving its informative purpose (in case somebody wants it to show up).

Figma link

Remaining tasks

Apply the new styles.

User interface changes

If shown, the block label will be integrated with the new designs.

CommentFileSizeAuthor
#26 label-padding.png20.18 KBahsannazir
block-label.png17.66 KBckrina
current-title.png41.8 KBckrina

Issue fork navigation-3415067

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ckrina created an issue. See original summary.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue summary: View changes

Adjusting summary.

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Assigned: Unassigned » kostyashupenko

kostyashupenko opened merge request !163

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Assigned: kostyashupenko » Unassigned
Status: Active » Needs review
finnsky’s picture

finnsky commented
Status: Needs review » Needs work

This code already defined.
Please check.

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented
Status: Needs work » Reviewed & tested by the community

Thanks, MR updated, very detailed answer written. Please check

finnsky’s picture

finnsky commented
Status: Reviewed & tested by the community » Needs review

That was planned as BEM block and not Drupal. Idk what else can appear in toolbar. Not sure we have to strictly relate them to Drupal entities here.

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented

New changes are based on BEM methodology aswell (sorry for wrong status)

finnsky’s picture

finnsky commented

New changes are based on BEM methodology aswell

I see here `toolbar-block` and `navigation-block`...

I can agree that they do not contradict the BEM, but they clearly do not follow it. I see that a second (duplicate) block has appeared just to replicate the Drupal structure. Which, it seems to me, is not final at the moment. So this new file seems like an unnecessary complication to me.

But I'll leave this for review by other community members.

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
commented

I would move `toolbar-block` BEM block on top of drupal's block. Again - we are rendering blocks with menus inside. Blocks can render label and content. So i'm thinking if `toolbar-block` classname should be added to `navigation-block.html.twig` as a wrapper. For menus inside - can be just `toolbar-block__menu` or `toolbar-block__section`.

It can be another task. I agree - this is all can be `toolbar-block` -> but it should start from drupal's block. Otherwise we have situation when we have drupal's block unnamed. And menu inside starts with `toolbar-block`, but should be an element of parent BEM block

ckrina closed merge request !163

ckrina reopened merge request !163

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Needs review » Needs work

This needs to be manually rebaded.

m4olivei made their first commit to this issue’s fork.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

I resolved the merge conflict.

ckrina closed merge request !163

ckrina reopened merge request !163

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented

I agree with @finnsky: both .navigation-block and .toolbar-block are the same component, and this MR keeps the 2 CSS files/components with the title in one and the rest of CSS in the other. But I agree with you @kostyashupenko that CSS and "PHP"-generated classes should match. Here's some the background behind the previous discussion regarding naming and the agreement we reached (before most of the back-end work started): #3387374 and #3386927.

Matching Drupal and BEM structure we have is:
- Navigation&co (wrapper, content...)
- Block (as Drupal block) and .toolbar-block<-- Here is where we need to add the label.
- Menu (as Drupal menu) and .toolbar-menu
- Menu links&co

On a back-end perspective we decided to use Navigation to avoid conflicts with the current Toolbar module. I know they don't match for now and they will have to match before opening a MR to core, but I wouldn't make naming changes in a specific MR that is supposed to only change styles. We'll need to properly discuss and plan the naming strategy now that we have both the front-end and back-end more defined, but let's have a discussion to properly bike-shed on that in another issue ;)

So let's keep the existing .toolbar-block for now and re-structure components or naming in a follow-up.

starshaped made their first commit to this issue’s fork.

starshaped’s picture

starshaped
she/her
Primary language English
commented
Status: Needs work » Needs review

Met with @ckrina today and discussed #19. I moved the title to be back in `toolbar-block.css` with the additions that were added.

finnsky closed merge request !163

finnsky reopened merge request !163

finnsky’s picture

finnsky commented
Status: Needs review » Needs work

Small issue with vertical spacing. Please take a look! All other looks great. Thank you!

ahsannazir made their first commit to this issue’s fork.

ahsannazir’s picture

ahsannazir commented
StatusFileSize
new label-padding.png20.18 KB

Removed the vertical padding as suggested because there is already ample spacing coming from grid-gap.

ahsannazir’s picture

ahsannazir commented
Status: Needs work » Needs review
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Needs review » Needs work

Since we've just merged #3402592: Reset theme css. and touches several CSS files, this needs to be rebased wit the latest changes.

ahsannazir’s picture

ahsannazir commented
Status: Needs work » Needs review
finnsky’s picture

finnsky commented

Looks good to me in frontend, but i would like to request backender review here. Since it contains some php.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Status: Needs review » Needs work

See MR comments. A couple of code changes are needed here.

ahsannazir’s picture

ahsannazir commented
Status: Needs work » Needs review
finnsky’s picture

finnsky commented

I see it not works now. Gonna fix

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented

Code looks good to me!

Left a minor comment in the MR.

finnsky’s picture

finnsky commented

There was some misunderstanding in begin so we had 2 definitions.
Now fixed

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented

Merged! Thanks all for the work here. This finished a really nice to have feature for the navigation!

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.