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.

Sign up for GitHub

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

Jump to bottom

Modernize UI Components #1022

Merged
merged 10 commits into from
Sep 13, 2019
Merged

Modernize UI Components #1022

merged 10 commits into from
Sep 13, 2019

Conversation

nummi
Copy link
Collaborator

@nummi nummi commented Sep 8, 2019 •
edited
Loading

Use @, this, on in place of action, angle brackets, tagName='', etc. for Inspectors UI add-on.

@nummi nummi changed the title WIP: Modernize UI Components Modernize UI Components Sep 8, 2019
nummi
nummi commented Sep 8, 2019
View reviewed changes
app/templates/application.hbs
@@ -28,11 +28,12 @@
<div class="split__panel">
<div class="split__panel__hd">
{{outlet "toolbar"}}
{{ui/sidebar-toggle
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Used only once so I removed it.

nummi
nummi commented Sep 8, 2019
View reviewed changes
app/templates/promise-tree-toolbar.hbs

<div class="toolbar__checkbox js-with-stack">
<label for="instrument-with-stack">
{{ui/action-checkbox
Copy link
Collaborator Author

@nummi nummi Sep 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Used only once so I removed it. (also, the way ActionCheckbox was implemented feels like an old pattern anyway)

Copy link
Member

Choose a reason for hiding this comment

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

🔥

nummi
nummi commented Sep 8, 2019
View reviewed changes
lib/ui/addon/components/error-page.js
@@ -3,12 +3,5 @@ import layout from '../templates/components/error-page';

export default Component.extend({
layout,

didInsertElement() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just use CSS for this, duh.

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain more about this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just use CSS to animate Tomster in place of manual DOM stuff.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, sounds good 👍

nummi
nummi commented Sep 8, 2019
View reviewed changes
lib/ui/addon/templates/components/error-page.hbs
</div>
<img class="tomster" src="assets/images/fishy_tomster.png" alt="Fishy Tomster">
</div>

<div class="error-page__body">
<div class="error-page__body-title">
{{#if bodyTitle}}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bodyTitle is never used by the app.

nummi
nummi commented Sep 8, 2019
View reviewed changes
lib/ui/addon/components/toolbar-search-field.js
import Component from '@ember/component';
import layout from '../templates/components/toolbar-search-field';

let uuid = 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Robert Jackson Suggested ™️

@RobbieTheWagner RobbieTheWagner self-requested a review September 9, 2019 20:08
@nummi nummi force-pushed the modernize-ui-components branch from 8182018 to 74f8ec7 Compare September 10, 2019 23:16
RobbieTheWagner
RobbieTheWagner requested changes Sep 10, 2019
View reviewed changes
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.

This is looking awesome! Just some general questions about potential simplification, but nothing too bad. Thanks for these PRs!

lib/ui/addon/components/toolbar-divider.js Outdated
@@ -0,0 +1,7 @@
import Component from '@ember/component';
Copy link
Member

Choose a reason for hiding this comment

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

Could we make this a template-only-glimmer-component and remove this JS file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure we can do that until 3.14. 🤷‍♀

Copy link
Member

Choose a reason for hiding this comment

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

We can, it has been a feature of https://github.com/emberjs/ember-optional-features for awhile. I think we need to run ember feature:enable template-only-glimmer-components then delete the JS file.

Copy link
Collaborator Author

@nummi nummi Sep 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The plan in my head was to made a pass at components like I did in this PR, then go over again and Glimmerize in another PR. Happy to give it a try now if you prefer.

Copy link
Member

@RobbieTheWagner RobbieTheWagner Sep 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I would just handle this one instance now, so we can get rid of the JS file, which we are only keeping around for tagName: ''. We can go through and find other things later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done and done.

lib/ui/addon/templates/components/toolbar-search-field.hbs Outdated
@@ -1,11 +1,19 @@
{{input value=value type="text" placeholder="Search"}}
<div class={{concat "toolbar__search " @wrapperClass}}>
Copy link
Member

Choose a reason for hiding this comment

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

Instead of @wrapperClass I think we can use normal class. Then I think if you pass class="foo" instead of @wrapperClass="foo" you should get both classes added to the element.

Suggested change
<div class={{concat "toolbar__search " @wrapperClass}}>
<div class="toolbar__search">

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was torn on this. If you were the end-user of <ToolbarSearchField /> would you think class would be applied to the <input>?

Copy link
Member

Choose a reason for hiding this comment

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

I think I would look at the template before assuming, so I think it should be fine to use class directly, rather than introducing a new variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Change made.

lib/ui/addon/templates/components/toolbar-search-field.hbs
title="clear"
{{on "click" this.clear}}
>
{{svg-jar "clear" width="14px" height="14px"}}
Copy link
Member

Choose a reason for hiding this comment

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

Is svg-jar angle bracket compatible? If so, could we use <SvgJar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

svg-jar is a helper.

lib/ui/addon/components/error-page.js
@@ -3,12 +3,5 @@ import layout from '../templates/components/error-page';

export default Component.extend({
layout,

didInsertElement() {
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain more about this?

app/templates/promise-tree-toolbar.hbs

<div class="toolbar__checkbox js-with-stack">
<label for="instrument-with-stack">
{{ui/action-checkbox
Copy link
Member

Choose a reason for hiding this comment

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

🔥

@RobbieTheWagner
Copy link
Member

@nummi looks like we have failures. Are they due to the component tree not being able to show template only / glimmer components?

@nummi nummi force-pushed the modernize-ui-components branch from e1cdd61 to c12f8ae Compare September 13, 2019 18:30
@nummi nummi force-pushed the modernize-ui-components branch from c12f8ae to 487c982 Compare September 13, 2019 21:27
@nummi nummi merged commit cf03a78 into emberjs:master Sep 13, 2019
@nummi nummi deleted the modernize-ui-components branch September 13, 2019 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RobbieTheWagner RobbieTheWagner RobbieTheWagner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nummi @RobbieTheWagner