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

Feature: Vertical Bar options #954

Merged
merged 80 commits into from Jun 17, 2020
Merged

Conversation

MitchellNZ
Copy link
Contributor

@MitchellNZ MitchellNZ commented Jun 3, 2020

Related to #763.
Does not address all issues linked to it. I have marked the ones that it does address.
Will most likely make a follow up PR to fix outstanding issues.

Looking for feedback and some assistance @stsrki!

Outstanding work

Theming
Breakpints:

  • Generate Breakpoints CSS in ThemeGenerator (this will replace _breakpoints.scss)

Bar:

  • Implement ThemeGenerator for sizing
  • Implement ThemeGenerator for colours (background, selected item, etc)
  • Selected item should have the Theme background (this should be done like GenerateDropdownStyles)

Documentation:

  • Bar Vertical modes.
  • Bar Collapse modes.
  • Bar NavigationBreakpoint.
  • New BarIcon.
  • How to create Right Align bar (see below).

Known issues

Ant Design Bar:

  • Bug: Selected/Active Item not highlighting
  • Bug: BarMode.VerticalPopout does not work
  • Bug: Selected/Active Item not highlighting
  • Bug: Fix Right Align Bar (needs CSS)
  • Bug: Fix clicking links on small bar (need to move icon inside link or something)
  • Bug: Hidden bar does not work
  • Bug: Small bar does not work

Future features

  • Dynamically created (code created) Bar
  • Third level dropdown menu (this works for inline, but not small or popout)
  • Support for Light/Dark themes with AntDesign

Note: I will open an issue (or multiple issues) to track these "future features" once merged

Additional notes for docs

[ADDED] How to: Right align sidebar

  • Put LayoutSider containing bar after Layout
  • Put RightAligned="true" on all BarDropdownMenu elements within the right bar

[FUTURE] How to: Merge BarBrand with topbar

  • Add box-shadow: none; style to vertical bar
  • Add bg-primary class (or equiv) to vertical BarBrand

Note: I will add this to the docs at a later time.. its a bit hacky and I want to find a better way

Summary

Let me know what you think @stsrki! You should have access to make changes - if not, let me know.
Otherwise some comments would be good and I can fix them up myself :)

If something in this list doesn't make sense to you, let me know and I can explain more.. these are just copied from my personal notes.
I will continue working on the known issues too!

# Conflicts:
#	Demos/Blazorise.Demo/Layouts/MainLayout.razor
@stsrki
Copy link
Collaborator

stsrki commented Jun 6, 2020

I will do ThemeGenerator sizing next. Please let me know if you need any more help.

Comment on lines 40 to 49
public override Task SetParametersAsync( ParameterView parameters )
{
// This is needed for the two-way binding to work properly.
// Otherwise the internal value would not be set in the right order.
if ( parameters.TryGetValue<bool>( nameof( Visible ), out var newVisible ) )
{
store.Visible = newVisible;
}

base.OnInitialized();
return base.SetParametersAsync( parameters );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stsrki Thank you for this! I couldn't figure this part out!

Will this no longer be needed once dotnet/aspnetcore#19642 is addressed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, component activator solves another problem. This is more of how Blazor works with parameters.

Actually, I had a similar problem with TextEdit a year ago so I just figured it might work.

@@ -88,6 +88,7 @@ protected override void OnParametersSet()

absoluteUri = To == null ? string.Empty : NavigationManager.ToAbsoluteUri( To ).AbsoluteUri;
active = ShouldMatch( NavigationManager.Uri );
DirtyClasses();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stsrki I had to add this line to get the active class appearing on the Link when its within the BarDropdownItem in AntDesgin (and only for AntDesgin!) I cannot figure out why this happening.. 🤔
Appears to be some sort of race condition with this method (OnParametersSet) and OnLocationChanged?
Seems like this may cause performance issues.. Maybe you could take a quick look, if you get a chance?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can override Link for AntDesign? I can try it later.

@stsrki
Copy link
Collaborator

stsrki commented Jun 17, 2020

The PR is really big but I think this should now be ready for merge. The only thing left is Bug: BarMode.VerticalPopout does not work and I'm not sure what is wrong with that. Maybe I'm looking at the wrong thing?

Anyways I will wait until you give me green light.

@stsrki
Copy link
Collaborator

stsrki commented Jun 17, 2020

These can wait until 0.9.2

  • Dynamically created (code created) Bar
  • Third level dropdown menu (this works for inline, but not small or popout)
  • Support for Light/Dark themes with AntDesign

We can open separate issue for each one.

@MitchellNZ
Copy link
Contributor Author

MitchellNZ commented Jun 17, 2020

The bug BarMode.VerticalPopout does not work is only for the AntDesign provider.
Its annoyingly quite difficult to get it working correctly! (Due to more playing around with custom CSS).

I think its probably best to merge this as it is and I'll create a follow up issue with this bug + the other things I plan on fixing for next release/patch. There are some things linked to the original issue (#763) I haven't had chance to fix as part of this either.

I don't want to be holding up this release anymore than I already have!
Does that sound good with you?

Edit: I agree, those Future Features will be for 0.9.2!

@stsrki
Copy link
Collaborator

stsrki commented Jun 17, 2020

That sounds great. Honestly this is more than I could imagine, you really did a great job with Bar component and I cannot thank you enough times.

So yeah, I will merge this now. And since I'm little stuck with other work I will not be able to finish release notes for 0.9.1 as fast as I thought. If you happen to do any other small task before release just create new PR. Otherwise we push anything left to 0.9.2.

And once again, THANK YOU!

@stsrki stsrki merged commit 1ac6a96 into Megabit:dev091 Jun 17, 2020
@MitchellNZ MitchellNZ linked an issue Jul 14, 2020 that may be closed by this pull request
@MitchellNZ MitchellNZ mentioned this pull request Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants