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

[Table] Add some props to patch css only style overrides #2246

Merged
merged 1 commit into from Nov 23, 2015

Conversation

alitaheri
Copy link
Member

These elements can only be styled through css classes. This PR temporary patches this component until we come up with a better solution than all these nested divs and tables. I won't add documentation since this is probably only temporary so people can have some way of overriding these elements without having to resort to css classes.

@@ -27,6 +27,9 @@ const Table = React.createClass({
selectable: React.PropTypes.bool,
style: React.PropTypes.object,
wrapperStyle: React.PropTypes.object,
headerTableInlineStyle: React.PropTypes.object,
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove the Inline part of the name to use the same convention as other component?
I also think that Table is not needed.
Could you also add those to the documentation?

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 kinda wanted the naming to be consistent with css classes, hence the inline.
The Table part is to avoid confusion with this:

<Table bodyStyle={...}>
...
  <TableBody style={...}>

But there are more cases like this around the library.

If you are ok with those I'll change the names and squash.

Copy link
Member

Choose a reason for hiding this comment

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

I kinda wanted the naming to be consistent with css classes

I think that the css classes shouldn't be here, let's forget that they exist.
Your example looks good to me.

@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Nov 23, 2015
@alitaheri
Copy link
Member Author

@oliviertassinari All done.

@oliviertassinari
Copy link
Member

Thanks. Could you also add them to the documentation (in an alphabetic order)?

@alitaheri
Copy link
Member Author

Alright I'll do that too. but then we'll have to support them for some time 😅

@alitaheri
Copy link
Member Author

@oliviertassinari Is this ok?

@oliviertassinari
Copy link
Member

but then we'll have to support them for some time

That's already the case for the className api. So I think it's better to have it documented. What better solution do you have in mind ?

@alitaheri
Copy link
Member Author

I'd say rewrite table, it's better not to use HTML table and instead do it all with div. This will give very flexible api and capability that would otherwise be impossible with table. But that is too far fetched. For now the least we can do is move the wrappers from Table to TableHeader, TableBody and TableFooter. If there are couplings between the parent and the child it should be resolved with carefully designed props instead of wrapping them within divs and tables. The latter hurts composibility and customizability.

But I guess if we decide to do this we have to rewrite all the table components. Therefore, deprecation will be very hard.

The best solution I can come up with is:

  1. Keep the Table as it is for now.
  2. Merge this PR as it is.
  3. Create new Table components.
  4. Deprecate the old components with a migration guide.
  5. Remove the old Table components after some time.

@oliviertassinari
Copy link
Member

@subjectix Thanks.
Regarding how we can improve the table component, I think that we should open a new issue to discuss this.

oliviertassinari added a commit that referenced this pull request Nov 23, 2015
[Table] Add some props to patch css only style overrides
@oliviertassinari oliviertassinari merged commit 7aac92a into mui:master Nov 23, 2015
@alitaheri alitaheri deleted the table-add-temp-props branch November 23, 2015 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

2 participants