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

Component to create: overlay #28

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

Component to create: overlay #28

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

Comments

@andrewconnell
Copy link
Member

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

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

@johnmeilleur
Copy link
Contributor

I will take this one on.

@johnmeilleur
Copy link
Contributor

SPEC

This one should be simple and I guess it is intended to be incorporated with other components (e.g. dialog)

main component directive: <uif-overlay></uif-overlay>
optional boolean attribute uif-overlay-dark. Defaults to false and when true adds the --dark class to the overlay.

examples:
<uif-overlay> Hello World </uif-overlay>
<uif-overlay uif-overlay-dark="true"> Hello World </uif-overlay>
<uif-overlay> <uif-dialog>...</uif-dialog> </uif-overlay>

@andrewconnell
Copy link
Member Author

IMHO I think this would make more sense:

<uif-overlay uif-mode="[light | dark]">Hello World</uif-overlay>

No need to include the name overlay in the attribute name as it's not going to be a directive. Also, one of the modes would be the default if omitted.

@johnmeilleur
Copy link
Contributor

I think that makes sense as well. I've submitted a pull request to review.

@andrewconnell
Copy link
Member Author

Since you're looking at an enum, the next merge of PR's to dev will introduce two new directive guidelines which will affect this directive. Specifically check guideline 007... it's in PR #94... guideline 008 is also related.

To be clear, theses aren't guidelines today, but we will be creating issues & assigning to the original directive authors in the coming days on stuff to address. So... when I review all the PRs in the next few days, if you don't have this no worries... we'll just create an issue and assign it to you, but if you want to be proactive, mentioning it now.

@johnmeilleur
Copy link
Contributor

Thanks. I'll review and update accordingly.

@waldekmastykarz
Copy link
Member

👍 on the spec @andrewconnell suggested

@johnmeilleur
Copy link
Contributor

I've submitted the PR to follow the guideline for attribute validation and error messages. I took a slightly different approach to injecting the logService. Not sure if one method is better then another.

@andrewconnell can you change the label of this issue to work in progress?

@johnmeilleur
Copy link
Contributor

Ready for review PR #103

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