Skip to content
This repository has been archived by the owner on Jan 16, 2018. It is now read-only.

ClayManagementToolbar #178

Merged
merged 46 commits into from
Nov 28, 2017
Merged

ClayManagementToolbar #178

merged 46 commits into from
Nov 28, 2017

Conversation

carloslancha
Copy link
Collaborator

@carloslancha carloslancha commented Nov 27, 2017

Missing features:

  • Quick action buttons
  • Search
  • Buttons on mobile
  • Plus button as a link and as a dropdown (This will be implemented as a component)
  • Columns selector (was not defined)

@carloslancha carloslancha changed the title Update demo ClayManagementToolbar Nov 27, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 83.456% when pulling 2452b14 on carloslancha:ClayManagementBar into 98f7b9d on metal:master.

@matuzalemsteles
Copy link
Collaborator

Just started reviewing :)

:octocat: Sent from GH.

"babel-loader": "^7.0.0",
"babel-plugin-transform-node-env-inline": "^0.1.1",
"babel-preset-env": "^1.6.0",
"browserslist-config-clay-components": "^1.0.0-alpha.3",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this not to be 1.0.0-alpha.2?

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, I changed, don't know why is again on alpha.3, I'm changing it!

* Total number of items.
* @instance
* @memberof ClayManagementToolbar
* @type {?int}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should to be {?number|undefined}? the default is undefined and validators is number.

* Number of selected items.
* @instance
* @memberof ClayManagementToolbar
* @type {?number}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should to be {?number|undefined}? the default is undefined.

* List of items to display in the actions menu on active state.
* @instance
* @memberof ClayManagementToolbar
* @type {?Array}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should to be {?array|undefined}? the default is undefined.

* List of filter menu items.
* @instance
* @memberof ClayManagementToolbar
* @type {?Array}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should to be {?array|undefined}? the default is undefined.

* @param {!Event} event
* @private
*/
handleSelectPageCheckboxChanged_(event) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

SF: Method alphabetically ordered

{@param? totalItems: number}

{let $navAttributes kind="attributes"}
class="management-bar management-bar-light navbar navbar-expand-md
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the default style when active should not be management-bar-primary?

</ul>

{if $actionItems}
<ul class="navbar-nav">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey @carloslancha, is not it necessary to add Quick actions? defined by the lexicon team?

screen shot 2017-11-28 at 11 35 09

Copy link
Collaborator

@matuzalemsteles matuzalemsteles Nov 28, 2017

Choose a reason for hiding this comment

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

I did not see the above comment, sorry 😅.


{if $filterItems}
<li class="dropdown nav-item">
{call ClayDropdown.render}
Copy link
Collaborator

Choose a reason for hiding this comment

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

On the mobile this will disappear. Did you get to see that? By markup, it is necessary to add a second button...
screen shot 2017-11-28 at 11 39 52

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is added to the missing stuff too

* @type {?bool|undefined}
* @default undefined
*/
selectable: Config.bool(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could it be Config.bool().value(false)?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 83.456% when pulling 173f8a0 on carloslancha:ClayManagementBar into 98f7b9d on metal:master.

@matuzalemsteles
Copy link
Collaborator

matuzalemsteles commented Nov 28, 2017

Merged. Thanks!

@matuzalemsteles matuzalemsteles merged commit d628b9c into deprecate:master Nov 28, 2017
@carloslancha carloslancha deleted the ClayManagementBar branch November 29, 2017 19:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants