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

feat(tree-view): add tree view #282

Merged
merged 18 commits into from Jun 5, 2020
Merged

Conversation

MaximBalaganskiy
Copy link
Contributor

No description provided.

@bigopon
Copy link
Member

bigopon commented May 29, 2020

This is awesome 😁
Can you help give some visualization please. Maybe a link?

@MaximBalaganskiy
Copy link
Contributor Author

@bigopon
Copy link
Member

bigopon commented Jun 1, 2020

@MaximBalaganskiy is this ready to go, what else left todo?

@MaximBalaganskiy
Copy link
Contributor Author

I had no more plans for this atm

Copy link
Contributor

@ben-girardet ben-girardet left a comment

Choose a reason for hiding this comment

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

Very nice work @MaximBalaganskiy

Left a few comments / questions... Looking forward to give this component a try in real life and see how it goes.

import { UxTreeViewTheme } from './ux-tree-view/ux-tree-view-theme';

export class UxDefaultTreeViewConfiguration {
public theme?: UxTreeViewTheme;
Copy link
Contributor

Choose a reason for hiding this comment

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

I kinda like this idea to have the theme in the configuration. But giving it more thoughts I'm afraid it leads consumers the wrong way... Just want to highlight something here to discuss the "default style scenario".

Without the theme configuration, the original way to apply a default style to a component is through the core styleEngine.applyTheme() method. When we use this method without passing an element then the styles are applied as default in the <HEAD/>.

Now I see two benefits from using applyTheme():

  1. I think it has better perf as it only apply the theme once
  2. More subtle: in this case when we pass a theme to a component, it will only override the default theme. On the contrary, with the default theme property on the plugin, the component will either use the default, or use the binded property.

My conclusions:

  • I like to have the theme property as a configuration as it makes it super easy for consumers to set a default theme
  • I'm not so confident with the idea to have a either/or scenario for the default theme. I think that if I set a nice button theme as a default but for some reason I need to change only the color at one place, I would prefer to only pass theme.bind="{background: 'red'}" instead of a whole new theme
  • Seems to me that perf-wise the applyTheme() is better

What do you think ? Should we keep the theme configurable option but in the configure() we do a applyTheme() (as a postTask or something like that) instead of applying the default theme in the component constructor ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does create ambiguity. I would remove it. But leave properties like dense or variant and pass them to inputs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would even remove it from the config as long as StyleEngine is documented

import { UxTreeView } from './ux-tree-view';

export interface IUxTreeViewElement extends HTMLElement {
au: {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at the moment, just for future needs

@bigopon
Copy link
Member

bigopon commented Jun 5, 2020

Thanks @MaximBalaganskiy for the initial implementation 👍

@bigopon bigopon merged commit ad1c108 into aurelia:master Jun 5, 2020
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

3 participants