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

UI Components Add-On #942

Merged
merged 27 commits into from
Feb 27, 2019
Merged

Conversation

nummi
Copy link
Collaborator

@nummi nummi commented Feb 24, 2019

The goal of this PR is to move as much generic UI logic to another location and keep the main repo for business logic. This is a huge commit so I'll leave comments where I think they will be useful.

Step two could include: more tests and documentation, usage of a component playground like Storybook or Ember CLI Addon Docs.

This will also make life easier if we go down the Inspector Add-ons route.

@nummi nummi changed the title WIO: UI Components Add-On WIP: UI Components Add-On Feb 24, 2019
@nummi nummi changed the title WIP: UI Components Add-On UI Components Add-On Feb 24, 2019
@RobbieTheWagner
Copy link
Member

This is awesome work! I'll give it a thorough review this week at some point. Pretty swamped with some other things currently.

@import "sidebar_toggle";
@import "split";
@import "toolbar";
@import "warning";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Most of these were moved to the add-on — a few were no longer being used.

transform: rotate(-90deg);
}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All of this was rolled into the disclosure-triangle component

Copy link
Member

@RobbieTheWagner RobbieTheWagner left a comment

Choose a reason for hiding this comment

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

Just a few general questions, but overall looks great to me!

</div>
{{else}}
--
{{/if}}
{{/list.cell}}

{{#list.cell class="list__cell_size_small list__cell_value_numeric"}}
<span class="pill pill--not-clickable pill--small js-view-duration">{{ms-to-time model.value.duration}}</span>
Copy link
Member

Choose a reason for hiding this comment

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

No need for the not-clickable class?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It didn't seen to have any effect. The class name didn't really describe what it did anyway. Also, I just changed the base .pill class to have that "not-clickable" functionality which is just inheriting the parents cursor.


<div class="divider" aria-hidden="true"></div>

<div
class="toolbar__search toolbar__search--small js-container-instance-search"
Copy link
Member

Choose a reason for hiding this comment

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

No need for the toolbar__search--small class anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It only made the search smaller by 25px. ¯_(ツ)_/¯


isExpanded: false,

isRight: equal('side', 'right'),
Copy link
Member

Choose a reason for hiding this comment

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

No need for this right stuff anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as I can tell we never had a sidebar toggle for the left sidebar. We can add this back if we ever need it.

@RobbieTheWagner RobbieTheWagner merged commit bd5219f into emberjs:master Feb 27, 2019
@nummi nummi deleted the ui-components-addon branch February 27, 2019 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants