Themes for yeti-table #16

bgantzler · 2019-01-10T04:08:38Z

Adds themes for css classes to be added to YetiTable.

Implements most of #7

bgantzler · 2019-01-10T04:18:43Z

Do you have any idea what the build error is? Everything works fine on my pc. yarn upgrade does not change my package.json.

miguelcobain · 2019-01-10T14:11:47Z

Seems like some dependency expected node 6 but we defined node 8 on dependencies?

cah-brian-gantzler · 2019-01-10T15:45:58Z

As far as I can tell its still allowing node 6 from the package.json, https://github.com/bgantzler/ember-yeti-table/blob/master/package.json#L85, its not anything that I can tell I changed

cah-brian-gantzler · 2019-01-10T15:52:13Z

I did update the dependancies for ember-decorators to 3.1.5

cah-brian-gantzler · 2019-01-10T16:03:11Z

OK rereading it I think its the reverse. More than likely ember-decorators 3.1.5 does NOT support 6, and 6 is in the package.json. Is it acceptable to remove node 6 from the engine list? I am not sure how to get this kind of warning locally so I can fix it before pushing

cah-brian-gantzler · 2019-01-13T21:32:16Z

I dont know what the problem is. The only think I thought it could be was I upgraded ember-decorators to 3.1.5. which has node 6* in the engines list. So I rolled that back, still the same error.

Error says that ip-regex 3.0.0 requires node =>8 that is true. I pulled the master branch and yarn says its installing ip-regex 3.0.0 so master branch should not compile for the same reason.

Not sure where to go from here except remove 6.* from the engines list. Which I am going to try now just out of curiosity of the build will even pass.

cah-brian-gantzler · 2019-01-13T21:36:39Z

Removing node 6 from the package.json resulted in the build erroring out because it seems to run on node 6 and ember-yeti-table itself doesnt match.

reverting

cah-brian-gantzler · 2019-01-14T04:24:49Z

It appears this is affecting other people and I think it is coming in from ember-cli-addon-docs. If you look for jan 12 messages in discord dev-ember-cli you will see talk about it. They are saying build with --ignore-engines or use yarn resolutions to mitigate for the time being. Ill see about getting a resolution and try again.

cah-brian-gantzler · 2019-01-14T12:38:35Z

reference ember-cli/ember-cli#8360

miguelcobain · 2019-01-14T16:31:35Z

Tried to add --ignore-engines. Let's see if that does the trick.

miguelcobain · 2019-01-14T17:40:50Z

Seems like adding --ignore-engines did the trick. Now we're back to the previous errors:

Error: Failed to execute 'setAttribute' on 'Element': '...attributes' is not a valid attribute name.

I think this happens because ember-angle-bracket-invocation-polyfill does not work when using the hbs template function in tests.
However, I tried running the app with ember 2.18 and it works fine. It's just tests that fail. So I'm not really sure on what to do about this one.

cah-brian-gantzler · 2019-01-14T17:46:35Z

I looked back in the build history and noticed that this error started sometime back on your master branch before I even started contributing.

So figured it was something to do with some dependency that you may have updated.

cah-brian-gantzler · 2019-01-15T02:54:18Z

Where go from here? Need some documentation updated? Tests?

miguelcobain · 2019-01-23T00:22:38Z

@cah-briangantzler I updated ember-decorators and a bunch of other things on master. Could you please rebase this PR?

cah-brian-gantzler · 2019-01-24T02:41:34Z

They removed @required? I found that useful when I was coding.

Being that you are now based on a beta, that changes may be a breaking change, if this is used in any application also using decorators, they could end up with a version that would be missing required and have the wrong path for types

I have rebased the code and re-uploaded

miguelcobain · 2019-01-24T02:46:30Z

They removed @required? I found that useful when I was coding.

Any @argument usage is now "required" by default, unless it specifies optional in its type.

they could end up with a version that would be missing required and have the wrong path for types

This shouldn't happen because yeti-table should have ember-decorators/argument version it specifies and the host app should have the other. It's like any other npm dependency. Someone correct me if I'm wrong.

Still, I will bump the minor version because this update will imply that only ember 3.4+ will be supported.

@pzuraq is trying to polyfill things into 2.8 LTS, but until/if that happens, yeti-table will be 3.4+ (people can always use older versions).

miguelcobain · 2019-01-24T02:48:12Z

addon/components/yeti-table/body/component.js

@@ -48,6 +49,9 @@ const arrayOrPromise = unionOf(
 class Body extends Component {
  layout = layout;

+  @argument(Object)


It seems this should be @argument('object')

miguelcobain · 2019-01-24T02:48:25Z

addon/components/yeti-table/body/row/cell/component.js

@@ -17,6 +17,9 @@ import layout from './template';
 class Cell extends Component {
  layout = layout;

+  @argument(Object)


miguelcobain · 2019-01-24T02:49:11Z

addon/components/yeti-table/body/row/cell/template.hbs

@@ -1,5 +1,5 @@
 {{#if column.visible}}
-  <td class="{{class}} {{column.columnClass}}" ...attributes>
+  <td class="{{class}} {{column.columnClass}} {{theme.tbodyCell}} " ...attributes>


No need to add an extra space here in the end of the class attribute.

miguelcobain · 2019-01-24T02:49:19Z

addon/components/yeti-table/body/row/component.js

@@ -49,6 +50,9 @@ import layout from './template';
 class Row extends Component {
  layout = layout;

+  @argument(Object)


miguelcobain · 2019-01-24T02:49:46Z

addon/components/yeti-table/component.js

+   *
+   * You may create your own theme-class and set `theme` to it's instance. Check Theme properties you may define in your own theme.
+   */
+  @argument(optional(Object))


miguelcobain · 2019-01-24T02:51:51Z

addon/components/yeti-table/foot/component.js

@@ -25,11 +26,19 @@ import layout from './template';
 class Foot extends Component {
  layout = layout;

+  @argument(Object)


miguelcobain · 2019-01-24T02:52:05Z

addon/components/yeti-table/foot/row/cell/component.js

@@ -23,6 +23,9 @@ import layout from './template';
 class Cell extends Component {
  layout = layout;

+  @argument(Object)


miguelcobain · 2019-01-24T02:52:22Z

addon/components/yeti-table/foot/row/component.js

@@ -26,6 +26,9 @@ import layout from './template';
 class Row extends Component {
  layout = layout;

+  @argument(Object)


miguelcobain · 2019-01-24T02:52:36Z

addon/components/yeti-table/head/component.js

@@ -29,6 +30,9 @@ import layout from './template';
 class Head extends Component {
  layout = layout;

+  @argument(Object)


miguelcobain · 2019-01-24T02:52:46Z

addon/components/yeti-table/head/row/cell/component.js

@@ -30,6 +30,9 @@ import layout from './template';
 class Cell extends Component {
  layout = layout;

+  @argument(Object)


miguelcobain · 2019-01-24T02:54:18Z

addon/components/yeti-table/head/row/column/component.js

@@ -47,6 +47,9 @@ import layout from './template';
 class Column extends DidChangeAttrsComponent {
  layout = layout;

+  @argument(optional(Object))


miguelcobain · 2019-01-24T02:54:30Z

addon/components/yeti-table/head/row/component.js

@@ -29,6 +29,9 @@ import layout from './template';
 class Row extends Component {
  layout = layout;

+  @argument(Object)


miguelcobain · 2019-01-24T02:54:42Z

addon/components/yeti-table/header/component.js

@@ -30,6 +31,9 @@ import layout from './template';
 class Header extends Component {
  layout = layout;

+  @argument(Object)


miguelcobain · 2019-01-24T02:54:54Z

addon/components/yeti-table/pagination/component.js

  ```
 */
 @classNames('yeti-table-pagination-controls')
 class Pagination extends Component {
  layout = layout;

+  @argument(Object)


cah-brian-gantzler · 2019-01-24T03:02:59Z

Ahh, makes sense on the required, I was looking through the docs and didnt see that.

@argument(optional(Object)) had been like that the while time, tests and everything run. Figured Object was like https://github.com/bgantzler/ember-yeti-table/blob/theme/addon/components/yeti-table/component.js#L147 Function isnt in quotes. Ill change them

I had to use the yarn resolve block for a while to get the correct version of another addon for a while. Thought it might fall under the same thing

miguelcobain · 2019-01-24T03:08:25Z

@cah-briangantzler I'm just following the docs here: https://github.com/ember-decorators/argument#primitive-types

For primitives types, the name should be provided as a string

And the list contains object and doesn't contain Function. Then I saw the definition of the Action type here: https://github.com/ember-decorators/argument/blob/master/addon/types.js#L14

export const Action = Function;

So I just used Function since in the @loadData case, it's semantically not correct to call it an action as it can be any function and not necessarily an action in the ember sense.

miguelcobain · 2019-01-24T03:19:31Z

addon/-private/themes/default-theme.js

+  columnSortedAsc: 'yeti-table-sorted-asc',
+  columnSortedDesc: 'yeti-table-sorted-desc',
+
+  pagination: {


My last concern is the pagination object.
With the current way this is set up, if we do something like:

<YetiTable @theme={{hash pagination=(hash info="some-class)}}>

all the other properties (pageSize, next and previous) would be overwritten be this new object.

One solution would be to always have a flat theme object structure, probably namespaced. e.g:

paginationInfo: 'yeti-table-pagination-controls-page-info', paginationSize: 'yeti-table-pagination-controls-page-size', paginationNext: 'yeti-table-pagination-controls-next', paginationPrevious: 'yeti-table-pagination-controls-previous'

But I do see how having some structure could help tidying up things a bit. In that case we would have to look for a way to "Object.assign" without overwriting the entire object. There are some packages on npm that do that.

What are your thoughts on this?

I think the original reason I was attempting Theme as an actual emberObject was to use mergedProperties in ember to solve this.

I prefer the organization of the nested objects and could see users arbitrarily adding nested objects to the config version and then trying to merge them, so it would be nice if it supported this.

Was searching and thought this one looked like it would do the trick https://davidwalsh.name/javascript-deep-merge (https://github.com/TehShrike/deepmerge)

That one looks great! deepMerge.all() seems to be what we're looking for.

With ember-auto-import in dependências we should be able to import it.

Ill see what I can do later. I thought about putting the 4 sort ones under a nest as well.

That sounds good.

…ion controls class should be themed as well. tests now refer to classes using the default theme object. add tests for deep merge.

miguelcobain · 2019-01-24T15:48:04Z

Ok, so I made some updates:

added deep merge
added tests for deep merge
moved sorting classes to a sorting object in theme
tests never refer any yeti-table-* classes directly and use default theme instead
use ember-code-snippet to place the default theme code in docs. This way it always stays updated
pagination controls container class wasn't themed

cah-brian-gantzler · 2019-01-24T16:29:50Z

Yea I noticed the pagination controls container, was going to fix it when I did the merge. Thanks for doing that. looking forward to the results.

miguelcobain · 2019-01-24T19:49:42Z

Thank you for your great work on this feature. 🎉

bgantzler force-pushed the theme branch from aece8f2 to fe4ae5b Compare January 13, 2019 21:23

bgantzler force-pushed the theme branch from fe4ae5b to dcb2e38 Compare January 13, 2019 21:32

bgantzler force-pushed the theme branch from dcb2e38 to 6ec6d6c Compare January 13, 2019 21:37

miguelcobain force-pushed the theme branch from 5dd81df to da3e84c Compare January 14, 2019 16:42

bgantzler and others added 2 commits January 23, 2019 21:22

Allows for a start of themes

9f81b18

update ember-cli-htmlbars-inline-precompile

f407268

bgantzler force-pushed the theme branch from c010ede to f407268 Compare January 24, 2019 02:24

Fixed a rebase error. Removed line that was not suppose to be there

d2b6ca5

miguelcobain reviewed Jan 24, 2019

View reviewed changes

Updated @argument(Object) to @argument('object')

4718fdf

miguelcobain reviewed Jan 24, 2019

View reviewed changes

miguelcobain added 2 commits January 24, 2019 15:43

add deep merge. refer to default theme using snippet in docs. paginat…

9bb60d6

…ion controls class should be themed as well. tests now refer to classes using the default theme object. add tests for deep merge.

fix remaining Object references in @argument

b452e94

remove unused import

cb7782b

bump ember-native-class-polyfill and allow 2.18 to fail

ee1edac

miguelcobain force-pushed the theme branch from abfd5c2 to ee1edac Compare January 24, 2019 19:48

miguelcobain merged commit 3536322 into miguelcobain:master Jan 24, 2019

miguelcobain mentioned this pull request Jan 29, 2019

Feature request: Make all yeti css classes configurable #7

Closed

Themes for yeti-table #16

Themes for yeti-table #16

Conversation

bgantzler commented Jan 10, 2019 • edited Loading

bgantzler commented Jan 10, 2019

miguelcobain commented Jan 10, 2019

cah-brian-gantzler commented Jan 10, 2019

cah-brian-gantzler commented Jan 10, 2019

cah-brian-gantzler commented Jan 10, 2019 • edited Loading

cah-brian-gantzler commented Jan 13, 2019

cah-brian-gantzler commented Jan 13, 2019

cah-brian-gantzler commented Jan 14, 2019

cah-brian-gantzler commented Jan 14, 2019

miguelcobain commented Jan 14, 2019

miguelcobain commented Jan 14, 2019 • edited Loading

cah-brian-gantzler commented Jan 14, 2019

cah-brian-gantzler commented Jan 15, 2019

miguelcobain commented Jan 23, 2019

cah-brian-gantzler commented Jan 24, 2019

miguelcobain commented Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cah-brian-gantzler commented Jan 24, 2019

miguelcobain commented Jan 24, 2019

miguelcobain Jan 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miguelcobain commented Jan 24, 2019

cah-brian-gantzler commented Jan 24, 2019

miguelcobain commented Jan 24, 2019

bgantzler commented Jan 10, 2019 •

edited

Loading

cah-brian-gantzler commented Jan 10, 2019 •

edited

Loading

miguelcobain commented Jan 14, 2019 •

edited

Loading

miguelcobain commented Jan 24, 2019 •

edited

Loading

miguelcobain Jan 24, 2019 •

edited

Loading