feat: issue #32 prev and next tree button #45
Conversation
|
@VWRoli how about the mobile version for this feature? |
src/Map.js
Outdated
| mountButtonPanelTarget.addEventListener('click', (e) => { | ||
| if (e.target.id === 'right-arrow') { | ||
| try { | ||
| this.goNextPoint(); | ||
| } catch (e) { | ||
| log.warn('go next failed', e); | ||
| } | ||
| } else if (e.target.id === 'left-arrow') { | ||
| try { | ||
| this.goPrevPoint(); | ||
| } catch (e) { | ||
| log.warn('go prev failed', e); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Do you think it is good that we move these lines to the button panel and expose: onClickNext, onClickPrev,
There was a problem hiding this comment.
I'm considering add an option to disablePrevNextButton
There was a problem hiding this comment.
I'm not sure what you mean. The way I did it exposes these functions? How should I go about not exposing them? This repo is a bit advanced for me, just started working on it for fun and for the challenge.
There was a problem hiding this comment.
@VWRoli no no you are doing great, I just want that you move the code above into the ButtonPanel, so the behavior of it gets encapsulated into the ButtonPanel component, I think it makes more sense, for example, I open a new option called:
new Map({disableButtonPanel})
then, in the current code, we need to take care of two places:
No1.
if(dsableButtonPanel){
new ButtonPanel()...
}
No.2:
if(disableButtonPanel){
buttonPanelTarget.addListner(....)
}
So it's getting cumbersome, in my mind, hiding the details (encapsulation) is the major benefit of using the component like ButtonPanel
There was a problem hiding this comment.
oh, maybe you need to expose events on the ButtonPanel:
new ButtonPanel({
onNext: this.goNextPoint,
onPrev: this.goPrevPoint,
})
There was a problem hiding this comment.
I understand now. I will see what I can do about it.
What do you mean exactly? Should I place them somewhere else on small screens? But this whole app does not have any mobile friendliness. |
@VWRoli yes, let see what happens, let's forget it now, we can open a new issue on this if needed. so, please ignore this. |
|
@VWRoli here is the design of these two arrow button, can you implement it? https://www.figma.com/file/XdYFdjlsHvxehlrkPVYq0l/Greenstand-Webmap?node-id=6133%3A21294 |
|
Yes, sure. |
VWRoli
force-pushed
the
ISSUE_#32_prev_and_next_tree_button
branch
from
f7360dc
to
5d6ef04
Compare
|
treetracker-web-map-core/src/Map.js Lines 830 to 837 in 41f6040 Could you take a look at it @dadiorchen? |
|
Good, thanks for the progress, yes, I will look into it, and I will back to you when I get any progress. |
|
@VWRoli thanks for you meaningful work on this, I finished this feature based on you work, take a look here: https://github.com/Greenstand/treetracker-web-map-core/pull/80/files |
The purpose of this PR is to fix #32