search

ngOfficeUIFabric / ng-officeuifabric

Office UI Fabric (https://github.com/OfficeDev/office-ui-fabric) implementation for Angular
http://ngOfficeUiFabric.com
MIT License
321 stars 67 forks source link

Component to create: commandbar #17

Closed andrewconnell closed 8 years ago

andrewconnell commented 8 years ago

Official link to component description & demo: http://dev.office.com/fabric/components/commandbar

Screenshot of web analytics for the original http://officeuifabric.com site by @andrewconnell. Use this to see what are the most popular components: http://imgur.com/gallery/buUcyQv

ghost commented 8 years ago

Hi Andrew, would love to get involved & help out with this having done a commandbar directive for my own benefit quite recently. Let me know if you're interested!

andrewconnell commented 8 years ago

hell yeah we're interested!!!! invite sent....

ghost commented 8 years ago

I've gone through the repo and made a start on this. I'm thinking of following a similar pattern to the dropdown directive in that there will be a directive for each commandBarItem and the commandBar itself. Should the overflow command be optional by parameter, or be baked in?

What (if anything), do you want me to do before getting too far into this?

andrewconnell commented 8 years ago

Start with a spec here...

See how other directives have a spec on the usage in their issue list like #36 or #14 or #35 ... how you implement it is really up to you... it's more that the signature is consistent with the other directives in the library.

ghost commented 8 years ago

Spec

<!-- commandbar -->
<uif-commandbar uif-overflow="true" uif-items="">
  <!-- commandbarSearch -->
  <uif-commandbar-search uif-placeholder="Search"/>
  <!-- main items -->
  <uif-commandbar-main ng-repeat="item in topnav.mainitems">
    <uif-commandbar-button uif-title="Move To"  uif-commandicon="ms-Icon--star" uif-showmore="true"/>
  </uif-commandbar-main>
  <uif-commandbar-side ng-repeat="item in topnav.sideitems">
    <uif-commandbar-button uif-title="Undo"  uif-commandicon="" uif-showmore=""/>
  </uif-commandbar-main>
</uif-commandbar>

Fabric allows for the inclusion of search bars, and for commandBar buttons to be positioned on the right. Search bar behavior is different in OneDrive, Video portal and http://dev.office.com/fabric/components/commandbar, for P1, will build a search bar which matches the behavior of http://dev.office.com/fabric/components/commandbar

Parameters

uif-items

defines the collection of commandbarButtons which can be passed in.

uif-overflow

defines whether the commandbar should display an elipsis (...) when viewport is <= @ms-screen-sm-max

uif-placeholder

defines the text placeholder in the search bar, defaults to 'Search...' when it's not defined

uif-title

defines the title of a commandbarButton

uif-showmore

if 'true', shows a downward chevron.

uif-commandicon

defines the icon to be used for the commandbarButton. if missing, then the commandbarButton will be a text-only button as per fabric CSS

Debate/Questions

Should sidecommand buttons be defined by an attribute of commandBarButton, or placed in another DOM element? As it's core to the search bar, I was intending to include the animation of the search bar, does this fit with the pattern?

Happy to discuss/adjust as appropriate.

waldekmastykarz commented 8 years ago

What's the difference between uif-items and

<uif-commandbar-main ng-repeat="item in topnav.mainitems">
  <uif-commandbar-button uif-title="Move To"  uif-commandicon="ms-Icon--star" uif-showmore="true"/>
</uif-commandbar-main>

I suggest we use uif-icon instead of uif-commandicon. Although we have the uif-icon directive, the uif-icon wouldn't collide because the directive can be used only as an element. Removing the command part would be consistent in how attributes are used in other directives.

Also I'd suggest we call the show more attribute uif-show-more rather than uif-showmore to consistently separate words as we do in other directives.

For the uif-placeholder attribute we could drop the uif- prefix: placeholder is an existing attribute in HTML which has the same function as uif-placeholder in this directive.

For the uif-title attribute I'm not sure if we should keep or drop the uif- prefix. Yes, there is a title attribute in HTML but in this case the contents of the uif-title attribute are not rendered as a title attribute but rather rendered as element contents. What do you think guys @ngOfficeUIFabric/ngofficeuifabric-members ?

waldekmastykarz commented 8 years ago

Re sidecommands: I'd vote for a separate element just as you included in your spec.

ghost commented 8 years ago

uif-items should be removed from the parameters list, I removed it from the spec but didn't tidy up the parameters list.

Agree with other comments regarding naming conventions - will await further feedback before adjusting spec

andrewconnell commented 8 years ago

For the icon, isn't the icon rendered just an <i>... like the normal icon? If so, is it not possible to reuse that? Regardless, make sure you use the enum in uif-icon for validation.

I'm not sure I get the sidecommand question... to me if it's part of the command bar, it should be a child to that element.

ghost commented 8 years ago

The icons are rendered using an <i>, and I'll reuse that.

Regarding the sidecommand, i'll keep it as a child element. Will update spec this evening

ghost commented 8 years ago

Updated, sorry for delay...comments welcome

Spec

<!-- commandbar -->
<uif-commandbar uif-overflow="true">
  <!-- commandbarSearch -->
  <uif-commandbar-search placeholder="Search"/>
  <!-- main items -->
  <uif-commandbar-main ng-repeat="item in topnav.mainitems">
    <uif-commandbar-button uif-title="Move To" uif-commandicon="ms-Icon--star">
    <i ng-if="showmore" class="ms-CommandBarItem-chevronDown ms-Icon ms-Icon--chevronDown"></i>
    </uif-commandbar-button>
  </uif-commandbar-main>
  <uif-commandbar-side ng-repeat="item in topnav.sideitems">
    <uif-commandbar-button uif-title="Undo"  uif-commandicon="">
    <i ng-if="showmore" class="ms-CommandBarItem-chevronDown ms-Icon ms-Icon--chevronDown"></i>
    </uif-commandbar-button>
  </uif-commandbar-main>
</uif-commandbar>

Fabric allows for the inclusion of search bars, and for commandBar buttons to be positioned on the right. Search bar behavior is different in OneDrive, Video portal and http://dev.office.com/fabric/components/commandbar, for P1, will build a search bar which matches the behavior of http://dev.office.com/fabric/components/commandbar

Parameters

uif-overflow

defines whether the commandbar should display an elipsis (...) when viewport is <= @ms-screen-sm-max

uif-placeholder

defines the text placeholder in the search bar, defaults to 'Search...' when it's not defined

uif-title

defines the title of a commandbarButton

uif-commandicon

defines the icon to be used for the commandbarButton. if missing, then the commandbarButton will be a text-only button as per fabric CSS

andrewconnell commented 8 years ago

thinking...

got a lot of feedback on this one... I'll follow up in the next 12hrs

andrewconnell commented 8 years ago

This isn't sitting right with me. I think my comment about the <i> tag were misunderstood, I meant you should leverage the uif-icon directive... one thing we're trying to do with directives is remove the mental baggage people using the fabric need to know with the classes they implement. This <i ng-if="showmore" class="ms-CommandBarItem-chevronDown ms-Icon ms-Icon--chevronDown"></i> goes against that.

When I look at the proposed spec, it's not jumping out at me at how things would be rendered.

I think there are some things that can be done to make it more in line with the how some other directives work. If the content of the button (title & icon) are in attributes, people can't style them how they see fit like with colors or sizes. If they were presented more like content, it would remove this limitation.

Looking at how the menu bar works in Angular Material, you have the top level container (they call it toolbar, we call it commandbar), then you'd have sections (like for search / sidebar / main area), and within the main area you have menus... each menu is a button with an option of additional buttons. When I look at how it works and compare it to fabric, it's pretty similar. Thoughts?

Consider...

<uif-commandbar uif-overflow="true">
  <!-- search control -->
  <uif-commandbar-search>
    <uif-searchbox />   <<< already implemented
  <uif-commandbar-search>
  <!-- main area -->
  <uif-commandbar-main>
    <uif-commandbar-menu>
      <uif-commandbar-item>
        <uif-commandbar-content>
          <uif-icon uif-type="alert" />First w/o submenu
        </uif-commandbar-content>
      </uif-commandbar-item>
      <uif-commandbar-item>
        <uif-commandbar-content>
          <uif-icon uif-type="save" />Second With submenu
        </uif-commandbar-content>
        <uif-commandbar-menu>
          <uif-commandbar-button />
          <uif-commandbar-button />
          <uif-commandbar-divider />
          <uif-commandbar-button />
          <uif-commandbar-button />
       </uif-commandbar-menu>
      </uif-commandbar-item>
      <uif-commandbar-item>
        <uif-commandbar-content>
          Third with no icon
        </uif-commandbar-content>
      <uif-commandbar-item>
    </uif-commandbar-menu>
  </uif-commandbar-main>
  <!-- side bar -->
  <uif-commandbar-side>
  </uif-commandbar-side>
</uif-commandbar>

This also reuses a few existing directives that you don't have to recreate & maintain: uif-searchbox & uif-icon

waldekmastykarz commented 8 years ago

:+1: on what @andrewconnell said. I think it would also be helpful to extend the spec with handling clicks on the different menu items.

One thing that I wonder about whether this implementation won't get overly complex: if you look at Office UI Fabric's HTML for commands, the label is wrapped in a separate span:

<a class="ms-CommandBarItem-link" tabindex="1">
  <span class="ms-CommandBarItem-icon ms-Icon ms-Icon--save"></span>
  <span class="ms-CommandBarItem-commandText ms-font-m ms-font-weight-regular">Delete</span>
</a>

If we structure the commandbar-item directive as follows:

<uif-commandbar-item>
  <uif-commandbar-content>
    <uif-icon uif-type="alert" />First w/o submenu
  </uif-commandbar-content>
</uif-commandbar-item>

won't it make it overly complex for us to wrap the First w/o submenu label in a proper span?

What do you think if we used instead:

<uif-commandbar-item>
  <uif-commandbar-content>
    <uif-icon uif-type="alert" />
    <uif-commandbar-text>First w/o submenu</uif-commandbar-text>
  </uif-commandbar-content>
</uif-commandbar-item>

It would also make it easier for us to tweak things in the future if necessary.

By the way: given that Office UI Fabric defines commandbar as CommandBar, shouldn't the directive be called uif-command-bar to be consistent with our naming conventions?

s-KaiNet commented 8 years ago

Jumping into conversation...
@andrewconnell you mentioned that this is possible to reuse uif-searchbox, but search box for CommandBar has different markup than SearchBox component. Looking into NavBar, there is also a search box which has third option for search box markup.
For me it's not possible to reuse it, every component will have its own search box, what is not very cool, but what other options?

waldekmastykarz commented 8 years ago

@s-KaiNet just an idea: if the functionality of the search component is the same across all the different places it's being used would be an option to dynamically set the template based on the parent directive (if any)?

s-KaiNet commented 8 years ago

I believe this is possible, but from what I see, markup is different, and this approach will look like single directive, but actually contains three other directives with completely different templates. Maybe it simpler to maintain it separately.

ghost commented 8 years ago

Hi all, thanks for feedback :+1:

@andrewconnell, agree that there is probably too much in the way of use of attributes, rather than elements, will change approach to mirror the rest of the project in that manner.

There are a few easy fixes (naming conventions, use of the uif-icon directive, and we should absolutely extend it to include clicks), but I think the search bar and sub-menu items are trickier.

Search

Regarding the search bar, I made a conscious decision not to use uif-searchbox because of the fact the markup is quite different. @waldekmastykarz is right in that the functionality is the same - being able to pass an html template in would get around this, but goes against the grain with how other directives have been built. Happy to be guided on this, think we either

a) alter uif-searchbox to allow for html templates b) implement uif-commandbar-search and take the hit with having to have similar code in the link/controller for the behaviour.

Submenus

Regarding submenus, before undertaking this I took a look through OneDrive, the Video Portal & Outlook to examine sub menu behavior (which isn't consistent) and concluded that submenus look to be using what would become other directives. For example...

Outlook Calendar

image

This looks to me like a ContextualMenu (http://dev.office.com/fabric/components/contextualmenu)

Video Portal

image

Looking at the DOM would suggest they're using ContextualMenu, with a few tweaks

Outlook mail is different again...looks to be some custom submenu

With that in mind, I left it with the user but agree that this adds some complexity for them, do we a) implement submenus with a child <uif-commandbar-menu> (Q: Does Fabric have the CSS for nesting submenus already, i've not seen this implemented in examples, or in the wild?) b) implement submenus with our own implementation of ContextualMenu/something inbetween? c) Leave it to the user? Least preferred really

Structure of commandbar-item

I would agree with @waldekmastykarz here, are others happy with this?

<uif-commandbar-item>
  <uif-commandbar-content>
    <uif-icon uif-type="alert" />
    <uif-commandbar-text>First w/o submenu</uif-commandbar-text>
  </uif-commandbar-content>
</uif-commandbar-item>

With the exception of the downward chevron for submenus, the icons are built in a slightly different manner in the CommandBar sample http://dev.office.com/fabric/components/searchbox, rather than using <i>, they use <span class="ms-CommandBarItem-icon ms-Icon ms-Icon--star">. I previously took this as gospel, hence my decision to implement uif-commandbar-button. However, having just tinkered with my fabric sample here, there is no impact in changing the element to be an <i>, so agree we should reuse <uif-icon> in both circumstances.

andrewconnell commented 8 years ago

I like the direction this is going now...

RE: wrapping the button text - yeah I see where we need to do that now... that stinks, but seems like we should.

RE: letting searchbox take in a template... not sure I like this if we're saying the consumer of ngOfficeUiFabric has to do it... if it's the directive that is doing it, then that's different & would make sense.

RE: submenus - I don't think we want to try to mimic how the dropdown menus get rendered from one of the implementations in Outlook / Video Portal / OneDrive... rather we want to mimic how fabric does it. I see the rendering differences in your pictures, but we want to stick with what fabric does, not deviate unless absolutely required.

Damn this one is more complicated than I earlier thought!

ghost commented 8 years ago

RE: letting searchbox take in a template - The directive (in this case the uifCommandBar) would be doing that, not the consumer.

RE: submenus - Does Fabric support submenus as part of the CommandBar in isolation? I didn't think it did, not at least without additional CSS. An additional .ms-CommandBarItem nested in a .ms-CommandBarItem within the main area creates a scrollbar within .ms-CommandBarMainArea. I referenced the O365 implementations as they seem to use another fabric component in order to create and manage submenus.

I know, I'm too far down the rabbit hole now!

andrewconnell commented 8 years ago

OK, so for now then (p1) if fabric doesn’t support submenus, then I’d just implement them without it. At this point I think we want to avoid introducing our own CSS if at all possible.

andrewconnell commented 8 years ago

Hold up... question out of the blue...

What if the command bar used the uif-contextual-menu directive for the menu parts that dropped down, just nicely positioned? I mean... how different is the submenu in that directive than uif-command-bar?

This is pretty drastic for a few reasons:

(1) clearly it would deviate from fabric... fabric has explicit classes for the submenus and if we did this, we'd be totally ignoring those, using the command bar for just the top-level stuff

(2) the uif-contextual-menu directive doesn't provide support for control over the text formatting or icons... after this comment i'm going to raise an issue over there

ghost commented 8 years ago

We might have gone full circle(ish) with submenus :), to answer your points....

1) I don't think this deviates from fabric because a) AFAIK, fabric doesn't have classes for CommandBar submenus b) Using uif-contextual-menu for the submenus seems to fit with how MSFT have implemented them, suggesting that it isn't/wasn't their intention for there to be submenu items which have the same style as the CommandBar icons (having submenu items which are the same height as the commandbar would look odd IMO anyway)

2) Not sure this is a disaster, icons in the uif-contextual would look odd and, again - not sure that was ever MSFT's intention.

If I've missed something regarding CommandBar submenu items in Fabric, it would be great if you can point me in the right direction, I may not be able to see the wood for the trees.

EDIT: to me, using uif-contextual-menu for submenus in the uif-command-bar makes sense. Reuse is always good and it seems to work pretty well in O365 implementations thus-far.

andrewconnell commented 8 years ago

I see your point. Any input from @ngOfficeUIFabric/ngofficeuifabric-members before @tobiaswest83 wants to get cracking on this one?

@tobiaswest83 would you post an updated spec to your current plan?

ghost commented 8 years ago

Quick update - I'll be updating spec on here in the next few days, not forgotten about this :)

andrewconnell commented 8 years ago

Awesome... thanks! Just making sure it was still being worked on since we hadn't seen a PR or heard anything.

ghost commented 8 years ago

uifCommandBar Spec

<!-- uifCommandBar -->
<uif-command-bar>
  <!-- uifCommandBarSearchbox -->
  <uif-command-bar-searchbox placeholder="Search"/>
  <!-- uifCommandBarSide side commands  -->
  <uif-command-bar-side>
    <!-- uifCommandBarButton, just text -->
    <uif-command-bar-item>
      <uif-command-bar-content>
        <span>With Icon</span>
      </uif-command-bar-content>
    </uif-command-bar-item>
    <!-- uifCommandBarButton, with an icon-->
    <uif-command-bar-item>
      <uif-command-bar-content>
        <uif-icon uif-type="plus" />
        <span>With Icon</span>
      </uif-command-bar-content>
    </uif-command-bar-item>
  </uif-command-bar-side>
  <!-- commandBarMain -->
  <uif-command-bar-main>
    <!-- command button -->
    <uif-command-bar-item>
      <uif-command-bar-content>
        <uif-icon uif-type="alert" />
        <span>First w/o submenu</span>
      </uif-command-bar-content>
    </uif-command-bar-item>
    <!-- uifCommandBarOverflow -->
    <uif-command-bar-overflow></uif-command-bar-overflow>
  </uif-command-bar-main>
</uif-command-bar>

uifCommandBarOverflow

defines the overflow elipsis and binds additional behavior to the menu when the viewport is reduced to a certain size.

uifSearchbox

There is sufficient difference in both the dom and the functionality (fly in/out of the input box and lack of a search button) of the commandBar searchbox I believe to actually justify a specific directive. Are you comfortable with this @andrewconnell?

uifCommandBarItem, uifCommandBarContent & uifCommandBarText

Each directives template defines core structure, user can add additional CSS classes appropriate to define font weight, color etc.

Notes

andrewconnell commented 8 years ago

Regarding the uif-command-bar-searchbox, if it’s that much different then it makes sense to not reuse what we have and have it be specific to this directive. However with that being said we should make sure that this one has the same public signature and validation as the other. A developer working with either one should make it seem like they are one in the same.

Question… why is there a uif-command-bar-text directive? What's the need for it over just span ?

ghost commented 8 years ago

It allows the user to define colors for either the icon or the element separately. I also found adding a fabric color class (.ms-fontColor-orangeLight for example) to the wrapper element only affects the color of the icon, not the span. Hope that makes sense?

If you're happy I'll progress some more with the directive and aim to get a PR over in the coming days

andrewconnell commented 8 years ago

But can't you do the same thing with:

      <uif-command-bar-content>
        <uif-icon uif-type="alert" />
        <span class="ms-fontColor-OrangeLight">First w/o submenu</span>
      </uif-command-bar-content>

Without having to use a custom text directive?

ghost commented 8 years ago

the uifCommandText template will be a span with fabric classes, e.g. <span class="ms-CommandBarItem-commandText ms-font-m ms-font-weight-regular">Categories</span>

If we use a span instead, consumers would be required to put the appropriate fabric classes in to ensure it rendered as fabric intended

andrewconnell commented 8 years ago

Can that not be achieved through using the postLink function in a directive? Within that function you can look at the rendered HTML and add the extra classes to the span if necessary as a last step.

I might be wrong... my motivation here is to limit as much work as possible for the user.

ghost commented 8 years ago

You could used a postLink and I'm happy to take your lead on this, but as a user they would need to either place their text in a <span> or <uif-command-bar-text>, I'm not sure we're creating work for them and using tags with the uif prefix seems to be consistent with other components I've looked at in the samples.

Sorry, I realise this has had a lot more debate than quite a number of other components..hope we're getting there.

andrewconnell commented 8 years ago

Hey no worries… better to make sure the spec is all agreed upon as it makes implementation easier for you & less rework / debate after the work has been done.

Curious… where did you see this in other directives? We definitely want to be consistent across directives, and the text will need to be wrapped in something because you can also have an icon or image. From my POV using an OOTB tag is easier than using a custom tag… but let’s be consistent with other directives.

ghost commented 8 years ago

Thanks :)

I think uifTableCell is similar if I recall.

I'll get started in the meantime

andrewconnell commented 8 years ago

In that case the contents of the cell go in there… so that makes sense. But you can include a SPAN and other stuff in there too if I’m not mistaken. That directive is likely going to have to get some significant overhaul as the fabric guys have said they are going to redo it to be an HTML table.

ghost commented 8 years ago

OK makes sense, we'll go with a span in place of uifCommandBarItemText and ensure the appropriate fabric classes are bound in the postLink. I'll update the spec and get cracking tomorrow, it's getting late here in the UK. Thanks Andrew

s-KaiNet commented 8 years ago

For contextual sub menu, If it possible, can you please skip implementation or wait for a while (2-3 days) until navbar will be completed. It appears that contextual menu needs some modifications to work easily with other directives. I'm going to make changes in the contextual menu, and as I'm the author, that will be easier to do it by my own.

ghost commented 8 years ago

How are you getting on with navbar? I've finished what I can for now bar an odd quirk with the behaviour when switching view ports and writing of the tests. I'll just need to make some tweaks to the overflow menu once uifContextualMenu has been updated.

s-KaiNet commented 8 years ago

I've pushed pull request here - https://github.com/ngOfficeUIFabric/ng-officeuifabric/pull/195. It's not merged yet, optionally you can grab my copy from here
The point of interest is NavBarItemDirective directive. Take a look how it uses contextual menu.