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

[docs] Fix build:docs command #6606

Merged
merged 1 commit into from
Apr 14, 2017
Merged

[docs] Fix build:docs command #6606

merged 1 commit into from
Apr 14, 2017

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Apr 14, 2017

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation next labels Apr 14, 2017
@@ -377,4 +377,3 @@ class Tabs extends Component {
}

export default withWidth()(Tabs);
export { Tabs as PureTabs }; // for testing purposes
Copy link
Member Author

Choose a reason for hiding this comment

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

@shawnmcknight Here we are.

Copy link
Contributor

Choose a reason for hiding this comment

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

@oliviertassinari Got it - thanks! I was trying to dive() against the rendered <Tabs> but failing because it returned a function. I see why your way is working and mine wasn't. I appreciate the insight! 👏

Copy link
Contributor

Choose a reason for hiding this comment

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

On second look, it appears (although you did it far cleaner than in my attempts) that I was actually using dive() correctly. It's your changes to withWidth that allowed this to work. Either way, glad to have learned something today 😀

| centered | bool | false | If `true`, the tabs will be centered. This property is intended for large views. |
| children | node | | The content of the component. |
| className | string | | The CSS class name of the root element. |
| fullWidth | bool | false | If `true`, the tabs will grow to use all the available space. This property is intended for small views. |
| index | number | | The index of the currently selected `Tab`. |
| index | number | | The index of the currently selected `BottomNavigation`. |
Copy link
Contributor

Choose a reason for hiding this comment

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

@oliviertassinari Out of curiosity, why was this line changed back to the original form? I had assumed the usage of BottomNavigation instead of Tab was an oversight, but maybe I am mistaken.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, in looking further, I missed updating the same doc in Tabs.js. Now I see that the docs generator is rebuilding the docs from the components. Silly me, I created the docs by hand 😧 .

I'll get a PR in for fixing Tabs.js and rebuilding the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can fix that here, thanks for raising it.

@oliviertassinari oliviertassinari merged commit 34a2bb1 into next Apr 14, 2017
@oliviertassinari oliviertassinari deleted the next-build-docs branch April 14, 2017 16:25
@muibot muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Apr 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants