Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should CollapseExpandButtons component be built into site-nav? #2299

Closed
ang-zeyu opened this issue Apr 27, 2023 · 7 comments
Closed

Should CollapseExpandButtons component be built into site-nav? #2299

ang-zeyu opened this issue Apr 27, 2023 · 7 comments
Assignees
Labels
c.Feature 🚀 p.High To be done in the next few releases
Milestone

Comments

@ang-zeyu
Copy link
Contributor

ang-zeyu commented Apr 27, 2023

Please confirm that you have searched existing issues in the repo

Yes, I have searched the existing issues

Any related issues?

#655

What is the area that this feature belongs to?

Author Usability

Is your feature request related to a problem? Please describe.

#2206 (comment)

Describe the solution you'd like

#2206 (comment)

Describe alternatives you've considered

No response

Additional context

No response

@ang-zeyu ang-zeyu added p.High To be done in the next few releases c.Feature 🚀 labels Apr 27, 2023
@ang-zeyu ang-zeyu added this to the v5.0.0 milestone Apr 27, 2023
@tlylt
Copy link
Contributor

tlylt commented Apr 29, 2023

That's a good point.

We might want to look at this in conjunction with #2081 (and the PR #2170). One benefit of "component-ifying", besides the ability to optionally exclude it, is the ease of further extending it to include customizable features. For example, the scroll-to-top has some customizability that can be adjusted (perhaps more conveniently).

The same thing can be said for future updates of "CollapseExpandButtons", which will make a single inline attribute toggle of this feature less extensible. Right now we do not have such implementation and therefore building into site-nav or not is in my opinion OK both ways.

@tlylt
Copy link
Contributor

tlylt commented Apr 29, 2023

Ping @MarkBind/active-devs for opinion.

@jovyntls
Copy link
Contributor

Maybe relevant: #2294 - if we decide to implement #2294, there may be some confusion with having two expand/collapse components. This may be a possible consideration for un-componentifying CollapseExpandButtons so that we can scope it to the site nav.

@tlylt
Copy link
Contributor

tlylt commented May 21, 2023

@ang-zeyu

  • any comment/feedback on the few mentioned points above? If not we will go as discussed to do the revert before release to move this issue forward
  • I see that Allow setting the width of search bar #665 is mentioned to be related, may I know how are they related? As in we should have done this in a similar fashion as searchbar?

Also, @itsyme any thoughts? Since you implemented the feature.

@ang-zeyu
Copy link
Contributor Author

  • If not we will go as discussed to do the revert before release to move this issue forward

    Sounds good, it would take some time to change or discuss this at least.

    Some other points are that:

    • We can have it as a component but place the restriction it must be inside the <site-nav> in the future, similar to how slots for panels and the like work (have a simple attribute based option but also a more flexible one). That should give more customization possibilities if needed.
    • Having it as a component outside the <site-nav> will allow one button to control multiple <site-nav>s. not sure if its a worthwhile use case, imo, we should still go with the intuitive and user friendly option until the need actually arises.
  • Sorry, this should be SiteNav: give a way to expand/collapse all items #655

@itsyme
Copy link
Contributor

itsyme commented May 22, 2023

@ang-zeyu

  • any comment/feedback on the few mentioned points above? If not we will go as discussed to do the revert before release to move this issue forward
  • I see that Allow setting the width of search bar #665 is mentioned to be related, may I know how are they related? As in we should have done this in a similar fashion as searchbar?

Also, @itsyme any thoughts? Since you implemented the feature.

Imo the best option is to have it as a component but inside <site-nav> as @ang-zeyu mentioned as I feel it would be the most intuitive for users as well!

@itsyme itsyme self-assigned this Jun 3, 2023
@tlylt tlylt modified the milestones: v5.0.0, v6.0.0 Jul 16, 2023
@yucheng11122017
Copy link
Contributor

Closed with #2303

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Feature 🚀 p.High To be done in the next few releases
Projects
None yet
Development

No branches or pull requests

5 participants