Skip to content

Tabgroup dropdown button size#2038

Closed
petermonky wants to merge 2 commits intoMarkBind:masterfrom
petermonky:fix/dropdown-size
Closed

Tabgroup dropdown button size#2038
petermonky wants to merge 2 commits intoMarkBind:masterfrom
petermonky:fix/dropdown-size

Conversation

@petermonky
Copy link
Contributor

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Resolves #2037

Overview of changes:

Adjusts size of tabgroup buttons in tabset to match that of non-tabgroup buttons.

Clickable region for non-tabgroup button Clickable region for tabgroup button
image image

Anything you'd like to highlight/discuss:

nil

Testing instructions:

Confirm clickable region by hovering over tabgroup and non-tabgroup buttons in rendered page.

Proposed commit message: (wrap lines at 72 characters)

Unify tabset sizing across tabgroup and non-tabgroup buttons


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@tlylt
Copy link
Contributor

tlylt commented Nov 15, 2022

Hi @ang-zeyu, seems like there's a problem with netlify preview that might need your help to configure on the netlify deployment page

6:47:23 PM: Build ready to start
6:47:23 PM: ---------------------------------------------------------------------
UNSUPPORTED BUILD IMAGE

The build image for this site uses Ubuntu 16.04 Xenial Xerus, which is no longer supported.

To enable builds for this site, select a later build image at the following link:
https://app.netlify.com/sites/markbind-master/settings/deploys#build-image-selection

For more details, visit the build migration guide:
https://answers.netlify.com/t/please-read-end-of-support-for-xenial-build-image-everything-you-need-to-know/68239


@ang-zeyu
Copy link
Contributor

Thanks for letting me know, https://deploy-preview-2038--markbind-master.netlify.app/ should work now. I sent you and @jonahtanjz an invite email as well in case.

Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @petermonky for tackling this issue 👍

Just one concern that I've commented on.

<slot name="button">
<a
class="dropdown-toggle"
class="dropdown-toggle nav-link"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component is also used by the navbar to create dropdowns.

As dropdown component in navbar already has nav-link class, the padding is applied here twice.

Notice how the links are no longer aligned on the navbar due to having extra paddings.
image

image

The relevant code:

<navbar type="primary">
  <!-- Brand as slot -->
  <a slot="brand" href="/" title="Home" class="navbar-brand">MarkBind</a>
  <li><a href="/userGuide/components/navigation.html#navbars" class="nav-link">Highlighted Link</a></li>
  <!-- You can use dropdown component -->
  <dropdown header="Dropdown" class="nav-link">
    <li><a href="#navbars" class="dropdown-item">Option</a></li>
  </dropdown>
  <!-- For right positioning use slot -->
  <li slot="right">
    <a href="https://github.com/MarkBind/markbind" target="_blank" class="nav-link">Fork...</a>
  </li>
</navbar>

Copy link
Contributor Author

@petermonky petermonky Nov 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhh yes this is an issue, let me see if I can find a workaround for this 👍

Copy link
Contributor Author

@petermonky petermonky Nov 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @jonahtanjz, I think it is okay to simply remove the nav-link classname from the dropdown element to remove the extra padding. i.e.

<navbar type="primary">
  <!-- Brand as slot -->
  <a slot="brand" href="/" title="Home" class="navbar-brand">MarkBind</a>
  <li><a href="/userGuide/components/navigation.html#navbars" class="nav-link">Highlighted Link</a></li>
  <!-- You can use dropdown component -->
  <dropdown header="Dropdown"> <!-- class="nav-link" removed -->
    <li><a href="#navbars" class="dropdown-item">Option</a></li>
  </dropdown>
  <!-- For right positioning use slot -->
  <li slot="right">
    <a href="https://github.com/MarkBind/markbind" target="_blank" class="nav-link">Fork...</a>
  </li>
</navbar>

the dropdown element element already wraps its anchor element with a nav-link anyway so I think the outer nav-link is safe to remove. I think this way the navbar elements would also be more consistent as the classname is only applied to the anchor elements within the navbar elements, dropdown or not:

image

I noticed that this would also affect the text colour of the anchor element and have accounted for it with another commit.

does this fix work, or is there a better approach?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this approach is fine but one main concern is that currently, most of the MarkBind sites will have the nav-link class in the dropdown component. If we go with this approach, users will have to remove the class on their end in the next release, which will be considered sort of a "breaking" change in the UI.

One workaround that you can do for now is to have an overriding CSS within Dropdown.vue to remove the padding from nav-link on the outer <li> element. This should ensure that there will not be extra padding with or without the nav-link class on the dropdown component. Once we are ready to make a release with breaking changes, we can then remove this workaround.

Note: You will need to update the user guide as well under navbar to remove the class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if removing the nav-link class is the best way to go. Because that means that it won't use the bootstrap styling anymore and may not be consistent with the other elements that use the Bootstrap styling.

Should I still proceed to remove it anyway? @jonahtanjz

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
This is how tabs look like with the removal of the nav-link class from dropdown looks. Some styling can obviously be done but not sure it will be better than using the in built bootstrap

@tlylt
Copy link
Contributor

tlylt commented Feb 12, 2023

Hi @petermonky, checking if you are still keen to implement the last few comments/fixes as requested?

@tlylt
Copy link
Contributor

tlylt commented Feb 28, 2023

Hi @MarkBind/contributors, please feel free to lend a hand here if possible to help continue the work by OP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tabset dropdown button should be more easily clickable

5 participants