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.

Sign up for GitHub

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

Jump to bottom

Add v4 support for tables #3070

Merged
merged 2 commits into from Mar 22, 2018
Merged

Add v4 support for tables #3070

merged 2 commits into from Mar 22, 2018

Conversation

astronomersiva
Copy link
Member

@astronomersiva astronomersiva commented Mar 9, 2018 •
edited

PR for implementing #3055

  • Changed table-condensed to table-sm.
  • Changed table-inverse to table-dark. There was a confusion caused by the migration guide but I guess I will go fix it there too.
  • Brought back the wrapping div for responsive tables. There is again a conflict between the migration guide and the documentation.

jquense
jquense reviewed Mar 9, 2018
View reviewed changes
Copy link
Member

@jquense jquense left a comment

Choose a reason for hiding this comment

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

Getting there!

src/Table.js Outdated
[prefix(bsProps, 'striped')]: striped,
[prefix(bsProps, 'bordered')]: bordered,
[prefix(bsProps, 'condensed')]: condensed,
[prefix(bsProps, 'sm')]: sm,
Copy link
Member

Choose a reason for hiding this comment

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

this feels like a weird choice by the bootstrap team since there is only one size. I'm guessing they were leaving space for other sizes? in which case this might make more sense as a size prop, like in Button.

Copy link
Member

Choose a reason for hiding this comment

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

I think similarly with dark that should become variant='dark' like Button maybe

src/Table.js Outdated
if (responsive) {
let responsiveClass = 'responsive';
if (breakpoint) {
responsiveClass = `${responsiveClass}-${breakpoint}`;
Copy link
Member

Choose a reason for hiding this comment

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

What about making responsive a prop that can be true or a breakpoint? like: responsive='sm' or just responsive, @taion thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah! That would be a better choice in fact.

src/Table.js Outdated
@@ -10,32 +10,35 @@ import {
} from './utils/bootstrapUtils';
Copy link
Member

Choose a reason for hiding this comment

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

We just merged some changes to how we do styling, can you take a look a the Button component as an example and use createBootstrap instead of any utils from bootstrapUtils? thanks!

jquense
jquense reviewed Mar 9, 2018
View reviewed changes
www/src/pages/components/table.js Outdated
</h3>
<p>
Across every breakpoint, use <code>responsive</code> for horizontally
scrolling tables.
Copy link
Member

Choose a reason for hiding this comment

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

lets maybe add a Callout as well here that responsive tables are wrapping in another div

@astronomersiva
Copy link
Member Author

astronomersiva commented Mar 17, 2018 •
edited

@jquense I have pushed the suggested changes. Please take a look.

Also, apologies for the delay :)

jquense
jquense approved these changes Mar 18, 2018
View reviewed changes
Copy link
Member

@jquense jquense left a comment

Choose a reason for hiding this comment

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

Just that one small nit

src/Table.js Outdated
<table {...elementProps} className={classNames(className, classes)} />
);
if (responsive) {
let reponsiveClass = 'table-responsive';
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be `${bsPrefix}-responsive?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good pick!

taion
taion reviewed Mar 19, 2018
View reviewed changes
.gitignore Outdated
@@ -2,6 +2,7 @@
.DS_Store
npm-debug.log*
node_modules
.yarn
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is yarn cache. Felt like adding it here since there's going to be a good number of people using Yarn.

src/Table.js Outdated
<table {...elementProps} className={classNames(className, classes)} />
);
if (responsive) {
let reponsiveClass = `${bsPrefix}-responsive`;
Copy link
Member

Choose a reason for hiding this comment

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

"responsiveClass"

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, thanks! :) I have changed and pushed.

@jquense jquense merged commit c1ba9e2 into react-bootstrap:v4-support Mar 22, 2018
@jquense
Copy link
Member

jquense commented Mar 22, 2018

🎉 Thanks a ton!

@astronomersiva astronomersiva deleted the v4-support branch March 26, 2018 06:22
jeffreyroshan added a commit to jeffreyroshan/react-bootstrap that referenced this pull request Mar 26, 2018
@jeffreyroshan
Merge branch 'v4-support' into v4-modal
b6866c5 
* v4-support: (43 commits)
  quick fix-up on BreadcrumbItem (react-bootstrap#3090)
  Add v4 support for Jumbotron (react-bootstrap#3064)
  Breadcrumbs (react-bootstrap#3050)
  Update logo to svg (react-bootstrap#3088)
  navbar (react-bootstrap#3082)
  Forward ref (react-bootstrap#3080)
  Navs (react-bootstrap#3062)
  Add v4 support for tables (react-bootstrap#3070)
  Add v4 support for images and figures (react-bootstrap#3059)
  Set sideEffects to false for webpack 4 (react-bootstrap#3075)
  Style context (react-bootstrap#3046)
  Gatsby v2 (react-bootstrap#3045)
  fix readme, react-16-enzyme doc running (react-bootstrap#3061)
  update to babel 7 (react-bootstrap#3044)
  attribute docs (react-bootstrap#3035)
  Update dependencies (react-bootstrap#3020)
  Fix type in README.md
  Changed ProgressBar check to work with proxied components (react-bootstrap#2965)
  Improve custom overlays docs (react-bootstrap#2998)
  Release v0.32.1-docs.0
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taion taion taion left review comments

@jquense jquense jquense approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@astronomersiva @jquense @taion