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

Update Menu to v1 API #319

Closed
wants to merge 2 commits into from

Conversation

awei01
Copy link

@awei01 awei01 commented Jun 30, 2016

Semantic-UI allows menu items with the class header to be <div>. Change behavior to allow header property that will then render a <div>. If MenuItem has header and href prop, then, render <a> tag.

awei01 added 2 commits June 30, 2016 11:57
Semantic-UI allows menu items with the class "header" to be <div>. Change behavior to allow "header" property that will then render a <div>. If MenuItem has "header" and "href" prop, then, render <a> tag.
Correct syntax for list checks
@codecov-io
Copy link

codecov-io commented Jun 30, 2016

Current coverage is 88.33%

Merging #319 into master will increase coverage by 0.02%

@@             master       #319   diff @@
==========================================
  Files            62         62          
  Lines           821        823     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            725        727     +2   
  Misses           96         96          
  Partials          0          0          

Powered by Codecov. Last updated by 44c796f...556c5de

@levithomason
Copy link
Member

Thanks for the PR. The Menu component is scheduled to be updated to our v1 API pattern, see #269. The concern I have is that if I release this header update, it will be immediately over turned by the Menu API update.

The CONTRIBUTING.md guide details how to go about designing a component API. The Spec out the API section is of primary importance here. There are also a number of great example PRs to reference linked in #269.

Would you be interested in updating the Menu component's API entirely?

@awei01
Copy link
Author

awei01 commented Jun 30, 2016

Not sure what your timeline is, but I can certainly contribute. I'll start with specs. Closing this PR for now.

@awei01 awei01 closed this Jun 30, 2016
@levithomason
Copy link
Member

You'll see in the contributing guide that we'd love to have the spec right here in the PR. Feel free to re-open and use the PR description or comments for the spec! ;)

@awei01
Copy link
Author

awei01 commented Jun 30, 2016

Ok this is a huge component... I'll give it a whirl, but please forgive any glaring oversights.

@levithomason
Copy link
Member

If it seems like a bit much to bite off at once, I can whip up the first spec and we can iterate from there. LMK.

@awei01
Copy link
Author

awei01 commented Jun 30, 2016

Menu API Proposal

This is really just to get the ball rolling...

<Menu>
    ...
</Menu>
<div class="ui menu">
    ...
</div>

@awei01 awei01 reopened this Jun 30, 2016
@levithomason
Copy link
Member

Types

All the Menu Types will follow this pattern, so we can just spec the one and extrapolate it to all Types: secondary pointing tabular text vertical pagination.

<Menu secondary>
  ...
</Menu>
<div class="ui secondary  menu">
  ...
</div>

@levithomason levithomason modified the milestone: v1.0 Jun 30, 2016
@levithomason levithomason changed the title feat(MenuItem): adding 'header' attribute Update Menu to v1 API Jun 30, 2016
@levithomason
Copy link
Member

We should address #142 in this PR as well.

@awei01
Copy link
Author

awei01 commented Aug 5, 2016

@levithomason Sorry, but I won't be able to contribute as much time as I had hoped.

@levithomason
Copy link
Member

OK, thanks for the note. I'll close this PR for now. Let me know if you decide to pick it back up later.

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

Successfully merging this pull request may close these issues.

3 participants