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
Introduces MapProvider, mappify and MapComponent #195
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice, everyone. Only minor questions... and if possibke add tests. Feel free to merge once the docs are right / tests are included.
*/ | ||
static propTypes = { | ||
/** | ||
* The children of this group. Typically a set of `ToggleButton`s. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs are copy and pasted, I guess
*/ | ||
map: PropTypes.oneOfType([ | ||
PropTypes.instanceOf(OlMap), | ||
PropTypes.func |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume there is no way of actually telling apart any func from a true Promise, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe if we fix your following comment.
ready: true | ||
}); | ||
} else { | ||
props.map().then((map) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, surprised aboit the invocation of map here. or are we expecting a function that returns a Promise? This might be me not understanding Promises... if so just ignore this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently yes. But this makes no sense at all... I gonna check this.
* | ||
* Wrapped components will be checked against the activeModules array of | ||
* the state: If the wrapped component (identified by it's name) is included | ||
* in the state, it will be rendered, if not, it wont. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these docs correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No.
/** | ||
* The wrapper class for the given component. | ||
* | ||
* @class The VisibleComponent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs again
} = this.props; | ||
|
||
if (map) { | ||
mapDiv = <div className="map" id="map" {...passThroughProps} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the hardcoded id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be also passed as a prop and default could by id
…
@marcjansen Thx for the catches. Gonna fix these soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Really great!
I just added some minor notes.
|
||
/** | ||
* The map can be either an OlMap or a Promise that resolves with an OlMap | ||
* if your map is created asynchronusly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asynchronusly
-> asynchronously
map: PropTypes.oneOfType([ | ||
PropTypes.instanceOf(OlMap), | ||
PropTypes.instanceOf(Promise) | ||
]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prop map
should be marked as required.
/** | ||
* Returns the wrapped instance. Only applicable if withRef is set to true. | ||
* | ||
* @return {Element} The wrappend instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrappend
--> wrapped
/** | ||
* Class representating a map. | ||
* | ||
* @class The Map. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Map
-> MapComponent
* @method componentDidMount | ||
*/ | ||
componentDidMount() { | ||
this.props.map.setTarget('map'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make use of prop mapDivId
here.
mapDiv = <div | ||
className="map" | ||
id={mapDivId} | ||
{...passThroughProps} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the possibility to pass children
to the div
element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Solved.
This introduces the HOCs
MapProvider
theMappifiedComponent
(mappify
) and theMapComponent
.Short explanation:
MapProvider
adds a map to the context of the application.mappfiy
function grabs the map from the context and adds it as prop to the according component.MapComponent
just returns a div containing an OlMap.It also includes an example of how to use these three new Components together.
We should check/modify our examples and tests to make use of the
MapProvider
.Thx @dnlkoch and @ahennr for your support.