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

Update grid list #739

Merged
merged 9 commits into from
Jul 19, 2017
Merged

Update grid list #739

merged 9 commits into from
Jul 19, 2017

Conversation

Subtletree
Copy link
Collaborator

@Subtletree Subtletree commented Jun 27, 2017

  • Use camelCase attribute names
  • Use contextual component api.
  • Use composability mixins
  • Add basic render tests
  • Remove demo grid-list warning

Attribute names are now camelCased e.g rowHeight. However the media suffixes are still dasherized rowHeight-gt-xs. Thought this would be inline with the flex-gt-xs class names and easier to read imo than rowHeightGtXs. Let me know if you want it all camelCase though.

Completes #738
p.s sorry about 'remote-tracking branch' commits

* Use camelCase attribute names
* Use compassable component api.
* Use composability mixins
* Add basic render tests
* Remove demo grid-list warning
@v3ss0n
Copy link

v3ss0n commented Jun 28, 2017

Awesome

{{#paper-grid-tile class="blue" md-rowspan="2"}}
{{#paper-grid-tile-footer}}
{{#paper-grid-list
cols-sm="1" cols-md="2" cols-gt-md="6"
Copy link
Owner

Choose a reason for hiding this comment

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

Did you keep dash-cased attributes here on purpose? It is true that size prefixes (-sm, -xs, -lg, etc) usually are dash-cased. However, I think consistency wins here. Trying to think of a good api for this.

Let me know what you think.

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe we could take inspiration from https://flexi.readme.io/docs/attributes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, mentioned it in the PR note.

Looks to me that flexi's api is essentially what we have now except we can't use simple names like sm and md as we have to differentiate between 3 properties with media options instead of flexi's 1.

Only reasonable options I can think of are:
a) cols="1" cols-gt-xs="2"
b) cols="1" colsGtXs="2"
c) cols="1 gt-xs-2"

Happy with any of these

Copy link
Contributor

Choose a reason for hiding this comment

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

I find colsGtXs very hard to read

Copy link
Owner

Choose a reason for hiding this comment

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

@Subtletree how hard could it be to implement c) ?

* Now uses attrs like `cols=“1 gt-xs-2 gt-md-4”`.
* Re worked internals, using computed properties and `didUpdateAttrs`
instead of observers.
* Fixed `undefined` error messages when tile `colspans` weren’t
compatible with grid `cols`.
* Fixed `removeListener` to properly remove event listeners.
* Should be some performance increase as it no longer re calculates
everything all the time.
@Subtletree
Copy link
Collaborator Author

Ok ready for another review:

  • Now uses attrs like cols=“1 gt-xs-2 gt-md-4”.
  • Re worked internals, using computed properties and didUpdateAttrs
    instead of observers.
  • Fixed undefined error messages when tile colspans weren’t
    compatible with grid cols.
  • Fixed removeListener to properly remove event listeners.
  • Should be some performance increase as it no longer re calculates
    everything all the time.

@Subtletree
Copy link
Collaborator Author

Think I know what's causing these errors, update soon.

@Subtletree
Copy link
Collaborator Author

Passes locally 😕

@Subtletree
Copy link
Collaborator Author

This appears to be hitting the qunit problem in #694 also.

@bjornharrtell Looks like you've fixed it in #747? Hopefully once that is merged this will pass.

@miguelcobain
Copy link
Owner

Thanks a lot for this, @Subtletree !

@miguelcobain miguelcobain merged commit 551a63c into miguelcobain:master Jul 19, 2017
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.

None yet

4 participants