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

trying out a component base idea #21

Closed
wants to merge 2 commits into from
Closed

Conversation

flynnplatt
Copy link
Contributor

DO NOT MERGE

Place to discuss and test component inheritance strategies

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 54.115% when pulling 3acf6dd on component-base into 348088c on master.

@AaronPlave
Copy link
Contributor

I'm not entirely sure if having this base container strategy buys us anything we don't already have since right now you can just extend the existing component and override the render function. Or am I missing something? Also one potential common use case to think of is adding a button to the layer controls component – what's the best way of doing that from a cmc app dev perspective?

@flynnplatt
Copy link
Contributor Author

this particular test is more the idea of formalizing our approach to component re-use. Like the LayerControlBase wouldn't even work on its own because it doesn't connect to the redux state. It's basically us saying, "This component should do X,Y, and Z but we don't care what it looks like".

As to adding a component, this approach could help with that as it would create two files, one for all of the things a component will do, and another for how the component will look. But that may also end up being messy. Plus I don't know where else this sort of issue comes up in CMC

@AaronPlave
Copy link
Contributor

Gotcha. I think the layer menu is probably our most complex and layered (hah) component, not sure if many other components really run into this issue much. One other idea I had was breaking the render functions up (taking adding a button to layer control container as ex) so that you wouldn't have to copy paste a huge render function just to swap out the core layer control container for your own.

@flynnplatt
Copy link
Contributor Author

flynnplatt commented Feb 20, 2018

What about extending that idea with the multiple render functions with actual new components? Like,

import {LayerButtons} from "_core/components/LayerMenu";
export class LayerControl extends LayerControlBase {
  renderButtons(){
    return (<LayerButtons />);
  }
  render() {
    return (
      ...
      {this.renderButtons()}
      ...
    );
  }
}

That way you have the flexibility of replacing individual pieces of the rendered component and we still have isolated component pieces.

I mean, this can work without the LayerControlBase but ya know


LayerControlBase.mapDispatchToProps = function(dispatch) {
return {
mapActions: bindActionCreators(mapActions, dispatch)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we do split out the base component here do we want to pass in mapActions as a prop?

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 dunno. I like the idea of not having to deal with any part of state/dispatch mapping if you inherit from the base. But then we'd need a way to easily override actions downstream

@AaronPlave
Copy link
Contributor

Yup, that's what I was thinking. I could go either way on the base component idea.

@flynnplatt
Copy link
Contributor Author

closing this in favor of #69 #68 and other more concrete work

@flynnplatt flynnplatt closed this Jun 7, 2018
@flynnplatt flynnplatt deleted the component-base branch June 11, 2018 13:34
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.

3 participants