- Issue created by @ckrina
- Assigned to kostyashupenko
- Issue was unassigned.
- Status changed to Needs review
10 months ago 10:04am 17 January 2024 - Status changed to Needs work
10 months ago 1:46pm 17 January 2024 - Status changed to RTBC
10 months ago 6:50am 18 January 2024 - ๐ท๐บRussia kostyashupenko Omsk
Thanks, MR updated, very detailed answer written. Please check
- Status changed to Needs review
10 months ago 8:37am 18 January 2024 - ๐ท๐ธ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`.
- Status changed to Needs work
10 months ago 1:45pm 31 January 2024 - First commit to issue fork.
- ๐ช๐ธ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&coOn 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.
- Status changed to Needs review
9 months ago 7:23pm 7 March 2024 - ๐บ๐ธ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.
- Status changed to Needs work
9 months ago 10:09am 8 March 2024 - ๐ท๐ธ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.
- ๐ฎ๐ณ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 7:28am 11 March 2024 - Status changed to Needs work
9 months ago 9:53am 11 March 2024 - ๐ช๐ธ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.
- Status changed to Needs review
9 months ago 7:14am 12 March 2024 - ๐ท๐ธ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 1:12pm 12 March 2024 - ๐จ๐ฆCanada m4olivei Grimsby, ON
See MR comments. A couple of code changes are needed here.
- Status changed to Needs review
9 months ago 6:23am 13 March 2024 - ๐ช๐ธ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 - ๐ช๐ธ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 9:55am 13 March 2024 Automatically closed - issue fixed for 2 weeks with no activity.