backdrop / backdrop-issues

Issue tracker for Backdrop core.
145 stars 40 forks source link

Bartik's main menu sub-links look ugly #726

Closed ghost closed 1 year ago

ghost commented 9 years ago

The sub-links in Bartik's main menu look ugly. They are styled as 'tabs' but appear under the top-level tabs: firefox_screenshot_2015-02-18t01-18-04 000z ('Test 2' is a sub-menu item of 'Test')

I propose instead something like this: firefox_screenshot_2015-02-18t01-25-59 855z

Steps to reproduce:

image

klonos commented 9 years ago

Good catch! That must be something we missed. Neither D7 nor D8 Bartik themes show 2nd level links.

wesruv commented 8 years ago

... can we just say Bartik is too close to being deprecated and not put effort into this?

I think the problem is Bartik is ugly :stuck_out_tongue:

serundeputy commented 8 years ago

oh; poor bartik; :sadpanda: indeed; point taken though;

bd0bd commented 8 years ago

I do not think Bartik is ugly. Menu is ugly. This is why I try to use Nice Menus module now. Nice Menus module also has some problem - it is mobile related issue.

klonos commented 8 years ago

... can we just say Bartik is too close to being deprecated and not put effort into this?

1457 is targeted for 1.4.0 and so is #1361 so if none of them is pushed further back, then my logical reaction is "yes, lets not", but...

I think the problem is Bartik is ugly :stuck_out_tongue:

...my emotional reaction is "I think it's ok" (uglier than the new Basis theme, but not necessarily ugly) + I use it in some simple sites and intranet apps built on Drupal/Backdrop. Some others use it too, so I think that we should put some effort to it. People moving from old D7 websites will then have minor things to change in their design after the upgrade + they will get the bonus of a "pimped-up" Bartik with a few extra features. This will be an extra reason for them to make the move. If we tell them that in order to get nice-looking menus they'll also have to switch theme and adapt it to match their current design/branding (more changes and thus more effort/cost required), then they are less likely to make the switch. Plus the more cost thing is against our philosophy.

My 2c.

Graham-72 commented 8 years ago

I think that we should put some effort to it

I agree and am willing to contribute some effort, even if only a small contrib module. I use Bartik a lot, but with custom secondary menus and not those out of the box. Nevertheless I am very much looking forward to the new core theme coming soon as another design option.

wesruv commented 8 years ago

Cool, I can take a crack at this since I did the tabs in seven it should be pretty easy to port over.

Just wanted to make sure it's worth the effort and those are some excellent reasons to follow up with it.

bd0bd commented 8 years ago

You going to make it like the admin menu? Would be cool.

ghost commented 8 years ago

Making a responsive menu for narrow devices, à la adminimal_admin_menu is easy, but I've not yet found a good design for a multi-level menu on tablet size, touch devices (no mouse), compatible with small laptops, trackpad driven — at least for a "classic" theme bartik like (multi-column, explicit header / footer…) Example anyone ?

Graham-72 commented 8 years ago

Cool, I can take a crack at this

:+1: :+1:

bd0bd commented 8 years ago

Making a responsive menu for narrow devices

I mean only dropdown menu without responsiveness. Even this would be cool.

ghost commented 8 years ago

only dropdown menu without responsiveness

just pick up the css files of nice_menus, and you've got a good starting point…

bd0bd commented 8 years ago

No, this is not a problem. I know CSS. Also I use this module - this is the problem. I mean dropdown menu inegrated by default in the core so any people could make multilevel menu without additional work (installing modules etc).

mikemccaffrey commented 8 years ago

We definitely should fix how the second level tabs are rendering in Bartik, since that looks obviously broken. However, I'm thinking we should stick to something simple, like a row of links. We should move conservatively with this theme, since: 1) We are deprecating it as the default theme and shouldn't spend too much time improving it, and 2) it has been the default theme for quite a bit of time, so we don't want to break the styling of any backdrop sites that are already using it, or any drupal sites using it that want a simple upgrade to backdrop.

Make sense?

bd0bd commented 8 years ago

@mikemccaffrey, I think it would be better to have two modes - keep the current version and a possibility to switch to the new mode. In the Bartik theme settings or the menu block config to have this mode switcher.

wesruv commented 8 years ago

I like where you're going @mikemccaffrey. If I added new functionality and it caused problems I'd rather not bury a bunch of time in it.

@bd0bd if there's a desire for a fancier/continuing improvement for bartik it could be forked in contrib-land? I think having 'modes' and managing two sets of code in one theme would be a bit of a pain and not very 'core appropriate'.

docwilmot commented 8 years ago

Making a responsive menu for narrow devices

See [UX] Provide responsive drop-down menus out of the box. Would be great to hear opinions on this.

bd0bd commented 8 years ago

@wesruv, I think my offer is not bad. It is not something new or special solution, it is good practice within transition periods. Yahoo mail box had the option to use old mailbox design or switch to the new one. Sometimes when we make a new website design but old visitots can use the old version too.

bd0bd commented 8 years ago

@docwilmot, this is what I mean - switcher. People could use the old version or new one - dropdown menu exactly like the admin menu. https://cloud.githubusercontent.com/assets/1042283/12556490/1e7a151c-c354-11e5-9dbe-12e251b2898b.PNG

klonos commented 8 years ago

@mikemccaffrey yes, we are deprecating it from being the default front-end theme that comes with core, but we are not removing it from core AFAIK. That's for the exact same reason: it has been in core, people are still using it (and my guess is that they will keep using for simple site designs) and we don't want to break their sites. I bet you, once we implement drop-down menus, people will expect them to work with it. If they see a row menu instead of drop-down, you'll see issues in the queue about it.

jenlampton commented 7 years ago

I think we should leave this issue here since it's a good novice one for people who are CSS experts to tackle, but as Bartik is no-longer our front-end theme, it's definitely not high priority.

ghost commented 2 years ago

It's been over 4 years since the last activity here. Any objections to just closing this issue?

Also, this no longer happens out-of-the-box. You have to first enable tabs in Bartik's settings to see this bug: image

ghost commented 2 years ago

Closing due to lack of interest/need.

klonos commented 2 years ago

@BWPanda I don't think that we should be closing issues with confirmed bugs just because we have other priorities. This issue is also marked good first issue, so it might be fixed by a person that is looking to dip their toes into contributing to Backdrop.

I think that the only reasons bug reports should be closed should be:

RakeshPotnuru commented 2 years ago

@BWPanda, @klonos I think this issue is resolved

image
stpaultim commented 2 years ago

@RakeshPotnuru - Your screen shot shows the menu on mobile. This issue is about the desktop view.

However, I am also having trouble to recreate this issue. Here are the steps that I THOUGHT would recreate it.

1) Spin up site and enable Bartik and make it the default theme. /admin/appearance 2) In the theme settings for Bartik, enable rounded tabs /admin/appearance/settings/bartik

image

3) Create a test page and make it a child of "About"

image

HERE IS THE RESULT:

image

For a beginner to work on this issue, we'll need complete steps to reproduce. Without those, we should probably close this issue (in a few months) as I am unable to even verify that it is still a problem.

NOTE: I will remove the "Good First Issue" tag until we have better steps to reproduce. If I don't know how to reproduce this issue, a brand new contributor is unlikely to figure it out. If you update the steps to reproduce, then please add the "Good First Issue" back.

RakeshPotnuru commented 2 years ago

@stpaultim It's looking fine, I think it's fixed.

image
ghost commented 2 years ago

The issue still exists:

image

jenlampton commented 2 years ago

restoring tags, thanks @BWPanda! I copied your STR into the Original Post.

stpaultim commented 2 years ago

I am working on this issue with the Triplo team in our first Office Hours for folks on the other side of the globe.

stpaultim commented 2 years ago

image

The Triplo team was working on this issue tonight and we did an exploratory Pull Request to get additional feedback, because we're not sure about the best solution

1) We are not experienced with Hierarchical menu's and are not sure what the expected behavior is. Please check to see if this is working correctly. 2) We are not sure how this should interact with the breadcrumbs. Currently it is overlapping, which does not seem correct, but the only alternative we can think of is to push the breadcrumbs down, which also does not seem correct. 3) Also, we are not sure if this kind of fix is even possible, in a backwards compatible way.

We appreciate some feedback.

ghost commented 2 years ago

@stpaultim I would recommend putting each level of the hierarchy on a line below its parent, with siblings sitting next to each other (rather than below each other like you currently have).

stpaultim commented 2 years ago

@BWPanda - Actually, in my image "Kittens and Puppies" is a child of "Test Page" and not a sibling. We never tested sibling menu items. When I did test them right now, they also stacked the same way. :-(

Thanks for that feedback.

But also:

1) What happens to breadcrumbs? Do they get pushed down or covered up?

ghost commented 2 years ago

Ah OK, thanks for the clarification.

Things should be pushed down, not covered. The only time I expect things to be covered is with a drop-down menu, and here we've specifically chosen otherwise.

stpaultim commented 2 years ago

For the record, we looked at this same menu display in Basis and it does not behave as @BWPanda suggested, which means that we are going to hold off on this issue until we get further clarification.

We added this to the next Design/UX agenda.

ghost commented 2 years ago

After listening to the Design/UX meeting discussion about this issue, I agree with @jenlampton's suggestion for just fixing the styling of sub-links in this issue, then open another issue for discussing the display of hierarchical menus in general.

I didn't realise there were two separate issues going on here.

So here's what it currently looks like when on a page that has sub-links:

image

I think either removing the background of the sub-links:

image

Or adding padding to match to top-level links:

image

Would be an improvement.

Now that I've done that, it seems like the last screenshot is what it's supposed to look like. I achieved that by simply adding display: block; to Bartik's style.css in this selector: .l-header ul.menu li li a {}

stpaultim commented 2 years ago

@BWPanda - Thanks. Feel free to submit your own PR. Otherwise, I think we'll revisit this issue at our next office UTC5 office hours. We can at least make a little progress on this and your screenshots help.

yorkshire-pudding commented 2 years ago

@BWPanda - your third option definitely looks right.

irinaz commented 2 years ago

@BWPanda , third option looks correctly, thank you! @stpaultim , to our conversation in UX meeting - admin menu module provides standard design/UX for hierarchal menus

Hierarchical-menu
stpaultim commented 2 years ago

@bdalomgir provided a PR that makes sense to me, but I'm still not certain it's what other people expect. He has indented 2nd level menu items as opposed to breaking them out by themselves as in the admin menu.

I think this might be sufficient for this task, feedback wanted.

image

stpaultim commented 2 years ago

While we have been putting work into this issue, we do still have the problem of how to make changes like this without breaking existing sites. This problem might be obscure enough and broken enough, that fixing it is relatively safe. But, can we be sure that we are not breaking existing sites?

https://github.com/backdrop/backdrop-issues/issues/4167

indigoxela commented 2 years ago

I think this might be sufficient for this task, feedback wanted.

I think this is a slight improvement, but TBH it still looks broken. This is (yet another) topic where we should avoid possible side-effects on existing installs (which in the end prevents lots of improvements).

Personally, I never use Bartik. Not even with D7, and I'd never consider using it with Backdrop (after upgrading). It looks so ancient. So maybe in this special case we should really consider a bigger CSS change - this theme is only there to make upgrades from D7 easier, right? Or is anyone actually using it?

yorkshire-pudding commented 2 years ago

@stpaultim - I changed the menu setting to dropdown (from hierarchical tree) and it works as you would expect. chrome_pS0RmMmNms

yorkshire-pudding commented 2 years ago

So a significant improvement. I also never use it. I can't see how this would break sites.

RakeshPotnuru commented 2 years ago

Just leaving a suggestion. Add a hover effect.

stpaultim commented 2 years ago

I changed the menu setting to dropdown (from hierarchical tree) and it works as you would expect.

@yorkshire-pudding - This was only intended to effect menus using the Hierarchical tree. To my knowledge the dropdown menu worked fine from the start.

I think this is a slight improvement, but TBH it still looks broken.

@indigoxela This is a good point and I should probably have been clear - this was discussed at the last design/UX meeting and no one had any idea what this SHOULD look like in Bartik or if it would be possible to make that kind of change. The thought was that we should make it at least look less broken. Hence, the suggestions provided by @BWPanda here: https://github.com/backdrop/backdrop-issues/issues/726#issuecomment-1078574729

These were the best ideas we could come up, but I agree that all of them still look broken.

Just leaving a suggestion. Add a hover effect.

@RakeshPotnuru - Good thought.

yorkshire-pudding commented 2 years ago

@stpaultim - good point I can't think why anyone would use hierarchical tree menus in that location

irinaz commented 2 years ago

@stpaultim - I changed the menu setting to dropdown (from hierarchical tree) and it works as you would expect. chrome_pS0RmMmNms

This looks perfect.

bugfolder commented 1 year ago

Tested this with the STR:

image

Applied the patch:

image

Similar to @stpaultim's comment above, I think it's sufficient for the task, and does what is intended. So WFM.

Reviewed the CSS code. LGTM.

RTBC.

quicksketch commented 1 year ago

Wow, this is an old issue. I'm happy to have merged https://github.com/backdrop/backdrop/pull/4027 to substantially improve the output of Bartik.

0ebf474 by @bdalomgir, @BWPanda, @stpaultim, @irinaz, @indigoxela, @yorkshire-pudding, @RakeshPotnuru, @bugfolder, @bd0bd, and @klonos.