Skip to content
This repository has been archived by the owner on Nov 4, 2023. It is now read-only.

refactor(CLIMATE): create separate directive #706

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rchl
Copy link
Collaborator

@rchl rchl commented Apr 27, 2021

Experimenting with separating each tile's code into its own directive.

To summarize, the specific tile's code is separated into its own directive and HTML template.

The directive's controller contains tile-specific code and is exposed as ctrl in the template.

Since item and entity are bound to the isolated scope of the directive, there is no need to pass those from the tile's template anymore since the controller has access to them already. Only in case we want to call the APIs from the main controller (exposed as ctrl.main in the tile's template) we need to pass them. That part is a bit dirty right now but could be made better eventually.

Please don't merge yet. Still figuring out if this is the best way to do it.

Please have a look and provide any feedback @alphasixtyfive @akloeckner

scope: true,
scope: false,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if we needed to create extra scope for each tile. Maybe we did since we are doing stuff like ng-if="_foo = fn()" and that avoided possible overwrites. Will have to test how this works exactly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will this influence, if generic function from the main controller are available? I'm thinking of things such as cacheInItem or the function you aliased in "Temporary transitioning APIs".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

scope: true creates a new scope that inherits from the parent scope. So everything from the parent scope is still accessible. It can only affect things when setting properties (like in the case I've mentioned).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And scope: false of course runs in the same scope so everything is accessible.

Copy link
Contributor

@akloeckner akloeckner left a comment

Choose a reason for hiding this comment

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

I am by no means an expert in telling you how AngularJS should be set up. So, I can only give comments on the generic structure. Overall: I think, it's a good idea to separate the specialized tile code from the overall tileboard code. 👍 We should just make sure not to set up any "traps" in doing this. :-)

</div>
</div>
<div
ng-if="item.type === TYPES.CLIMATE"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we get rid of those specialized code sections completely in tile.html? Kind of include all tile templates from directory ...?

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 also thought that would look weird once all tiles are converted to have these repetitive elements here.

I think it could be solved with some directive that would dynamically insert a relevant directive based on the type. My angularjs is a bit rusty but it should be doable, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is good idea. This would also pave the way to completely custom tile types user can create themselves and share if they want.

@@ -0,0 +1,4 @@
import { App } from '../../app';
import tileClimate from './tileClimate';
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here: Can we generically include all tile types? Alternatively: We could add the html part of the template here, so we don't have two places to add a new tile type...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Importing all tile files at once might not be doable with ES imports since imports are hoisted and run before any code so we can't read files from disk and then import based on that. But we could do some preprocessing with rollup to generate files with everything imported, maybe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's of lesser importance and we should solve the html part first. (Maybe with said automatic/dynamic directives you mentioned.)

scope: true,
scope: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this influence, if generic function from the main controller are available? I'm thinking of things such as cacheInItem or the function you aliased in "Temporary transitioning APIs".

@akloeckner
Copy link
Contributor

Please don't merge yet. Still figuring out if this is the best way to do it.

Isn't there a "draft PR" feature for this situation?

@rchl rchl marked this pull request as draft April 28, 2021 07:47
@cgarwood
Copy link
Collaborator

Any more movement in this direction? I've been wanting to add some new tiles but the current structure makes that a bit difficult, so having them split out to separate files would be awesome.

@rchl
Copy link
Collaborator Author

rchl commented Jan 19, 2022

Sadly, I don't think I can currently devote that much time to get all of those refactors done.

Though I guess this PR might be "mergable" already, if we ignore the raised improvement suggestions. Maybe that would kick-start more changes from other people.

@cgarwood
Copy link
Collaborator

I'd certainly be up for helping split some more tiles out to separate files if there was a framework in place for it 🙂

@akloeckner
Copy link
Contributor

My 50ct: Yes, please ignore my suggestions, if they are blocking improvement! If there is interest in advancing this, we should go with the usual spirit... Improve step by step and handle problems if and when they arise.

So, Charles, if you are planning on adding some tiles, you will certainly stumble accross some of these issues. Meaning, they'd even be adressed in a timely manner. And by the time you're done with the new tiles, the framework will have settled to something consistent. I'd be happy to provide some more 50ct from my non-ng-expert perspective any time, if you like.

If we don't want this convergence to happen on the main branch, we could set up a feature branch and merge when appropriate. However, since the main branch receives updates only once per month by now, I would just wait for this convergence to settle before creating a new release...

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.

4 participants