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 theme #426

Merged
merged 7 commits into from
Oct 2, 2017
Merged

Table theme #426

merged 7 commits into from
Oct 2, 2017

Conversation

billy-addepar
Copy link
Contributor

Allow table to have custom theme.

@billy-addepar
Copy link
Contributor Author

@Addepar/ice

@pzuraq
Copy link
Contributor

pzuraq commented Sep 28, 2017

I think it's probably better to just allow users to implement their own styles using the provided class names

@billy-addepar
Copy link
Contributor Author

billy-addepar commented Sep 29, 2017

but they would have to override many properties in CSS class of our default theme. That's what I want to avoid. If I want to have custom theme, I would create it by my own without overriding default theme.

@pzuraq
Copy link
Contributor

pzuraq commented Sep 29, 2017

In general, addon/styles should only include the bare minimum of CSS required for the addon to function as there is no way for the user to exclude the styles from the build. If a user doesn't want our default styles, they probably also don't want to ship it to all of their users.

We can either include the styles in app/styles where they can be optionally imported, or better yet style it internally. We'll likely be creating a second addon to extract the table components we use, so that's the most natural place to add those styles.

@billy-addepar
Copy link
Contributor Author

Done.

I would rather move the style to app.scss than creating a new addon for it. Having an addon just for the style of this table is overkill.

@@ -19,7 +19,7 @@ const SELECTION_MODE_MULTIPLE = 'multiple';

export default class EmberTable2 extends Component {
@property attributeBindings = ['style:style'];
@property classNames = ['et-table'];
@property classNameBindings = ['tableClass'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this, we should just allow users to style it themselves, no need to add custom class names

@@ -57,6 +57,11 @@ export default class EmberTable2 extends Component {
@property selectionMode = 'single';

/**
* Theme of this table. Override this property to have custom theme for the table.
*/
@property tableTheme = 'et-default-theme';
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed

@@ -105,6 +110,12 @@ export default class EmberTable2 extends Component {

@property lastSelectedIndex = -1;

@computed('tableTheme')
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed

@@ -26,3 +26,70 @@
.custom-row {
background: green;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This should all be moved to /app/styles/ember-table/default.scss, not in the Dummy app. Check out how ice-pop and other internal addons work, this allows users to import the styles optionally.

@billy-addepar
Copy link
Contributor Author

Done

Copy link
Contributor

@pzuraq pzuraq left a comment

Choose a reason for hiding this comment

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

It should be under a subdirectory, otherwise it can overwrite other users styles. app/styles/ember-table/default.scss

@billy-addepar
Copy link
Contributor Author

Why does it override user's style? I thought for SASS, if you don't import it, it won't be used?

@pzuraq
Copy link
Contributor

pzuraq commented Sep 29, 2017

The contents of the app folder get directly merged into the main app. If there is another file named /app/styles/default.scss it will be overwritten.

@billy-addepar
Copy link
Contributor Author

Done

@billy-addepar billy-addepar merged commit 38d89fd into 2.0 Oct 2, 2017
@bantic bantic deleted the billy/table-theme branch July 11, 2019 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants