Skip to content
This repository was archived by the owner on Jun 14, 2018. It is now read-only.

Component to create: callout #15

Closed
andrewconnell opened this issue Dec 29, 2015 · 23 comments
Closed

Component to create: callout #15

andrewconnell opened this issue Dec 29, 2015 · 23 comments
Assignees

Comments

@andrewconnell
Copy link
Member

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

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

@jjczopek
Copy link
Contributor

jjczopek commented Jan 7, 2016

I would like to take this one as my next component ;)

@jjczopek
Copy link
Contributor

Due to it's characteristics, callout component should have four element directives:

  • main component directive: <uif-callout></uif-callout>
  • callout header directive: <uif-callout-header></uif-callout-header>
  • callout content directive: <uif-callout-content></uif-callout-content>
  • callout actions directive: <uif-callout-actions></uif-callout-actions>

Main directive <uif-callout></uif-callout>

  • callout tag: <uif-callout></uif-callout>.
  • optional attribute uif-callout-arrow which can take following values: left|right|top|bottom. If not specified default is left. Indicates arrow direction.
  • optional attributes uif-callout-separator and uif-callout-actiontext to indicate that there should be horizontal rule rendered just above callout actions. See callout component example.
  • optional attribute uif-callout-type for OOBE & Peek callouts. See OOBE & peek example. Valid values are OOBE|peek.
  • optional attribute uif-callout-close - indicating that the callout can be closed with X button in the top-right corner.
  • support for ng-show attribute directive, determining whether the callout is visible or not.

Header directive <uif-callout-header></uif-callout-header>

This is just to wrap the header content around proper HTML tags.

  • header tag <uif-callout-header></uif-callout-header>.

Header directive <uif-callout-content></uif-callout-content>

This is just to wrap the main content around proper HTML tags.

  • content tag <uif-callout-content></uif-callout-content>.

Header directive <uif-callout-actions></uif-callout-actions>

This is just to wrap the actions content around proper HTML tags.

  • content tag <uif-callout-actions></uif-callout-actions>.

Final notes

I think the above is a good starting point, but default component lacks interactivity. Therefore I think it would be good to split this componeny into two phases, similarilu as table component.

Second phase would be about introducing following enhacements:

  • allow callout to be closed with X mark in the callout corner.
  • support animations when callout is displayed - e.g fade in/fade out based on ng-show attribute changes.

Feedback is highly appreciated.

@andrewconnell
Copy link
Member Author

👍 I like this

@waldekmastykarz
Copy link
Member

👍 looks good

@jjczopek
Copy link
Contributor

Updated progress on the callout.

@jjczopek
Copy link
Contributor

@ngOfficeUIFabric/ngofficeuifabric-members, I need your input.

Callout can have something you could call a separator, just above actions element:
image 2

Basically that's just a CSS class that gets added to the main div element of the callout, nothing more. And the CSS class is called ms-Callout--actionText.

What I'm struggling with is:

  • Fabric is using term actionText for this element, but I can't see any relation to text,
  • Because it looks like a separator to me, I came up with custom attribute called uif-callout-separator, but it feels inconsistent with the Fabric way of naming it. It should rather be uif-callout-action-text or something.

Please let me know what do you think is better: keep it named separator (which gives better understanding what it does) or change to action-text (tohave it more consistent).

@andrewconnell
Copy link
Member Author

While I agree that we want to not deviate from Fabric's implementation / naming of things, I think in this case what you suggest makes a heck of a lot more sense. I'd name it uif-callout-separator.

Random - what do you think about implementing both? Would it be trivial or a lot of work to support either custom attribute & document it for users? Say "we used this because that's what it is, but for those familiar with the fabric css class, you can use this one... but be clear they do the same thing & if there's a conflict, this [x] one takes precedence"? I have no idea if this is introducing a lot of work for you... if so, nix it.

In general going forward, we don't want to implement something differently than how the team did it just because we think it could be done better. For instance, they have used jQuery to create some behaviors with their controls... we don't want to change the actual behavior while we're rewriting it in non-jQuery just because a menu should open a different way than they did it (which is also saying "we want our menu to open differently than Office 365"). In a case like that, we would possibly provide a way to change the behavior.

@jjczopek
Copy link
Contributor

There is not a lot of logic behind it, so I think supporting both makes a lot of sense.

@waldekmastykarz
Copy link
Member

If you look at the example at http://dev.office.com/fabric/components/callout it seems like the element wraps actions which explains why it's called actions rather than only separator.
screen shot 2016-01-12 at 19 23 19

@jjczopek
Copy link
Contributor

@waldekmastykarz all other examples have actions as well, but they don't have separator (due to lack of ms-Callout--actionText on the main callout div), and there is going to be directive for rendering this actions div with ms-Callout-actions class.

@waldekmastykarz
Copy link
Member

Check, I misunderstood it. So what if we could have the directive responsible for rendering the actions div add the ms-Callout--actionText class to div.ms-Callout? On the actions directive we could include an attribute such as uif-separator that would add the css class responsible for the separator.

@jjczopek
Copy link
Contributor

That also makes a lot of sense, and would not pollute the main directive so much with unnecessary logic. I'll wait a bit for other opinions, but I like that one.

@waldekmastykarz
Copy link
Member

👍

@jjczopek
Copy link
Contributor

Quick question to everyone:

When callout coponent can be closed, a button element with nested icon element is rendered - have a look here

Question: should I append this button as plain element - basically appending <button> tag with an <i> tag, or it would better to leverage uif-icon and uif-button for that?
Basically how do we feel about depending on other components that are inside the library already?

@ghost
Copy link

ghost commented Jan 14, 2016

My 2 cents: no dependencies. At this stage, yes, the HTML for the icon inside the callout component is the same as the separate icon. But this may change in the future. E.g. it may require an additional attribute/element/... which only makes sense in the callout.
I think Angular Material does have dependencies though, but at this stage I don't see the benefit for us.

@andrewconnell
Copy link
Member Author

My $0.02: I don't think it's bad to depend on other directives in the library that are already there. In fact, I think it's preferred. If things need to be split out later, then that's something that could be addressed.

@jjczopek
Copy link
Contributor

@andrewconnell ok, so I'm waiting for the uif-button directive from you then ;)

@jjczopek
Copy link
Contributor

Discussion point/advice needed.

If you take a look into the specification I made, I came up with three additional directives for header, content and actions. My intention when it comes to usage is something like that:

<uif-callout>
  <uif-callout-header>Header</uif-callout-header>
  <uif-callout-content>Content</uif-callout-content>
  <uif-callout-actions>Actions</uif-callout-actions>
</uif-callout>

If you have a look into Fabric examples, you will notice that inside the main div there is another div with class main (div.ms-Callout-main), however content and actions are enclosed in another div with class inner (div.ms-Callout-inner). Inner is child of div.ms-Callout-main, not the top-level callout div.

That means that using it the way I thought it should be used, would (by default) transclude everything within the div.ms-Callout-main. But content and actions have to be wrapped by div.ms-Callout-inner.

I identified potential solutions:

  1. Introduce yet another directive called uif-callout-inner and it would be required to wrap content and actions with that directive;
  2. Introduce postLink function to main directive and inspect child elements - in case content or actions found - create div.ms-Callout-inner and put it around content and actions element.

If you have better options, please let me know.

@ghost
Copy link

ghost commented Jan 19, 2016

Hmm.
I'd say option 2, as this would be the easiest / cleanest from a user point of view.

@andrewconnell
Copy link
Member Author

I don’t have strong feelings either way, but agree that #2 is slightly better from my POV.

@jjczopek
Copy link
Contributor

On Fabric, when callout has this separatot (ms-Callout--actionText), buttons inside actions container should also have (ms-Callout-action). Should this be driven by the directives used inside - e.g. whether button component or link component should have the responsibility to add those classes based on the context they are in? Or should that be callout component to check if such components are there and adjust them accordingly>

@waldekmastykarz
Copy link
Member

If these are generic buttons then I'd say that it's the responsibility of the separator to decorate them with specific classes that it requires for proper rendering. Otherwise we would introduce a dependency (even if optional) from buttons to callout which is odd.

andrewconnell pushed a commit that referenced this issue Jan 24, 2016
Add new directive `uif-callout`.

Closes #15. Closes #119.
@andrewconnell
Copy link
Member Author

Manually merged.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants