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

Tabs2 ➡️ Tabs #1900

Merged
merged 7 commits into from
Dec 15, 2017
Merged

Tabs2 ➡️ Tabs #1900

merged 7 commits into from
Dec 15, 2017

Conversation

giladgray
Copy link
Contributor

Tabs2 replaces original Tabs API.

@@ -1,5 +1,5 @@
/*
* Copyright 2015 Palantir Technologies, Inc. All rights reserved.
* Copyright 2017 Palantir Technologies, Inc. All rights reserved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 original date?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm down to leave the original date.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like to use 2015-present since it's never out of date

export const Home = () => <Tabs selectedTabIndex={0}>{contents}</Tabs>;
export const Projects = () => <Tabs selectedTabIndex={1}>{contents}</Tabs>;
export const Home = () => <Tabs selectedTabId="home">{contents}</Tabs>;
export const Projects = () => <Tabs selectedTabId="projects">{contents}</Tabs>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adidahiya are these updates correct? is this section still useful?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, we should probably remove this react router section because it probably doesn't work with react-router v4. we can add it back if requested by users.

@blueprint-bot
Copy link

and fix all the *other* usages

Preview: documentation

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

prefer to wait on #1885 and deal with merge conflicts here rather than there

@giladgray
Copy link
Contributor Author

@adidahiya - waited, resolved merged conflicts.

@blueprint-bot
Copy link

copyright year

Preview: documentation

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

lgtm, just remove the react-router part

export const Home = () => <Tabs selectedTabIndex={0}>{contents}</Tabs>;
export const Projects = () => <Tabs selectedTabIndex={1}>{contents}</Tabs>;
export const Home = () => <Tabs selectedTabId="home">{contents}</Tabs>;
export const Projects = () => <Tabs selectedTabId="projects">{contents}</Tabs>;
Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, we should probably remove this react router section because it probably doesn't work with react-router v4. we can add it back if requested by users.

@adidahiya
Copy link
Contributor

you probably need to merge master to fix the overlay test flake.

@blueprint-bot
Copy link

Merge branch 'master' of github.com:palantir/blueprint into gg/tabs

Preview: documentation | table

@giladgray giladgray merged commit 2888209 into master Dec 15, 2017
@giladgray giladgray deleted the gg/tabs branch December 15, 2017 00:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants