Add styles for block navigation label

Created on 16 January 2024, 10 months ago
Updated 27 March 2024, 8 months ago

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.

๐Ÿ“Œ Task
Status

Fixed

Version

1.0

Component

Code

Created by

๐Ÿ‡ช๐Ÿ‡ธSpain ckrina Barcelona

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @ckrina
  • ๐Ÿ‡ช๐Ÿ‡ธSpain ckrina Barcelona

    Adjusting summary.

  • Assigned to kostyashupenko
  • ๐Ÿ‡ท๐Ÿ‡บRussia kostyashupenko Omsk
  • Merge request !163Add styles for block navigation label โ†’ (Merged) created by kostyashupenko
  • Issue was unassigned.
  • Status changed to Needs review 10 months ago
  • ๐Ÿ‡ท๐Ÿ‡บRussia kostyashupenko Omsk
  • Status changed to Needs work 10 months ago
  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

    This code already defined.
    Please check.

  • Pipeline finished with Success
    10 months ago
    Total: 145s
    #79062
  • Pipeline finished with Failed
    10 months ago
    Total: 81s
    #79075
  • Pipeline finished with Success
    10 months ago
    Total: 154s
    #79079
  • Status changed to RTBC 10 months ago
  • ๐Ÿ‡ท๐Ÿ‡บRussia kostyashupenko Omsk

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

  • Status changed to Needs review 10 months ago
  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

    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.

  • ๐Ÿ‡ท๐Ÿ‡บRussia kostyashupenko Omsk

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

  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

    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.

  • ๐Ÿ‡ท๐Ÿ‡บRussia kostyashupenko Omsk

    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`.

  • Pipeline finished with Success
    10 months ago
    Total: 156s
    #84206
  • Pipeline finished with Success
    10 months ago
    Total: 169s
    #84281
  • Pipeline finished with Success
    10 months ago
    Total: 157s
    #84301
  • Pipeline finished with Success
    10 months ago
    Total: 161s
    #84309
  • Pipeline finished with Success
    10 months ago
    Total: 158s
    #84312
  • Pipeline finished with Success
    10 months ago
    Total: 164s
    #84814
  • Pipeline finished with Success
    10 months ago
    Total: 164s
    #84820
  • Pipeline finished with Success
    10 months ago
    Total: 143s
    #84821
  • Pipeline finished with Success
    10 months ago
    Total: 150s
    #84823
  • Status changed to Needs work 10 months ago
  • ๐Ÿ‡ช๐Ÿ‡ธSpain ckrina Barcelona

    This needs to be manually rebaded.

  • First commit to issue fork.
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada m4olivei Grimsby, ON

    I resolved the merge conflict.

  • Pipeline finished with Success
    10 months ago
    #88415
  • ๐Ÿ‡ช๐Ÿ‡ธSpain ckrina Barcelona

    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.

  • First commit to issue fork.
  • Pipeline finished with Success
    9 months ago
    Total: 193s
    #105215
  • Pipeline finished with Success
    9 months ago
    Total: 149s
    #114069
  • Status changed to Needs review 9 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States starshaped

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

  • Pipeline finished with Success
    9 months ago
    Total: 150s
    #114074
  • Status changed to Needs work 9 months ago
  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

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

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia ehsann_95

    ahsannazir โ†’ made their first commit to this issueโ€™s fork.

  • Pipeline finished with Success
    9 months ago
    Total: 193s
    #116217
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia ehsann_95

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

  • Status changed to Needs review 9 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia ehsann_95
  • Status changed to Needs work 9 months ago
  • ๐Ÿ‡ช๐Ÿ‡ธSpain ckrina Barcelona

    Since we've just merged โœจ Reset theme css. Fixed and touches several CSS files, this needs to be rebased wit the latest changes.

  • Pipeline finished with Success
    9 months ago
    Total: 146s
    #117016
  • Status changed to Needs review 9 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia ehsann_95
  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

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

  • Status changed to Needs work 9 months ago
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada m4olivei Grimsby, ON

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

  • Pipeline finished with Success
    9 months ago
    Total: 236s
    #117890
  • Status changed to Needs review 9 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia ehsann_95
  • Pipeline finished with Success
    9 months ago
    Total: 149s
    #117919
  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

    I see it not works now. Gonna fix

  • ๐Ÿ‡ช๐Ÿ‡ธSpain plopesc Valladolid

    Code looks good to me!

    Left a minor comment in the MR.

  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

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

  • Pipeline finished with Canceled
    9 months ago
    Total: 125s
    #117932
  • Pipeline finished with Canceled
    9 months ago
    Total: 36s
    #117933
  • Pipeline finished with Success
    9 months ago
    Total: 148s
    #117934
  • ๐Ÿ‡ช๐Ÿ‡ธSpain ckrina Barcelona

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

  • Status changed to Fixed 9 months ago
  • ๐Ÿ‡ช๐Ÿ‡ธSpain ckrina Barcelona
  • Pipeline finished with Success
    9 months ago
    #118025
  • Pipeline finished with Success
    8 months ago
    #125334
  • Pipeline finished with Success
    8 months ago
    Total: 53s
    #125389
  • Automatically closed - issue fixed for 2 weeks with no activity.

  • Pipeline finished with Canceled
    7 months ago
    Total: 1024s
    #160985
  • Pipeline finished with Success
    7 months ago
    Total: 1280s
    #160989
  • Pipeline finished with Success
    7 months ago
    Total: 1468s
    #161088
  • Pipeline finished with Success
    6 months ago
    Total: 1011s
    #173503
  • Pipeline finished with Skipped
    6 months ago
    #176317
  • Pipeline finished with Success
    6 months ago
    #181212
  • Pipeline finished with Success
    6 months ago
    Total: 165s
    #196167
  • Pipeline finished with Success
    6 months ago
    Total: 166s
    #196171
  • Pipeline finished with Success
    6 months ago
    Total: 137s
    #196173
  • Pipeline finished with Success
    6 months ago
    Total: 167s
    #196177
  • Pipeline finished with Success
    5 months ago
    Total: 139s
    #198240
  • Pipeline finished with Canceled
    5 months ago
    Total: 645s
    #198253
  • Pipeline finished with Failed
    5 months ago
    Total: 1115s
    #198258
  • Pipeline finished with Success
    5 months ago
    Total: 1049s
    #198314
  • Pipeline finished with Skipped
    5 months ago
    #198357
Production build 0.71.5 2024