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

Add support for ImageButton in ZoomToMaxExtentButton component #271

Merged
merged 1 commit into from
Nov 3, 2015

Conversation

nmco
Copy link

@nmco nmco commented Nov 3, 2015

Remove div around the image in ImageButton component

@simboss simboss added the ready label Nov 3, 2015
@nmco
Copy link
Author

nmco commented Nov 3, 2015

I improve a little bit (I hope) the ImageButton component: remove the div around the image tag and give the possibility to pass a custom style. I upgraded and updated all the need tests. I also give the possibility to the ZoomExtendButton component to support an ImageButton (the same behavior as the InfoButton).

},
getStyle() {
var style = {
var finalStyle = {
cursor: "pointer",
margin: 0,
padding: 0,
display: "inline-block"
};
if (this.props.image && this.props.image !== "") {
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 equivalent to: if(this.props.image) {

Copy link
Author

Choose a reason for hiding this comment

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

I will change that :)

@nmco
Copy link
Author

nmco commented Nov 3, 2015

By the way, a pattern is emerging for the buttons: being a normal (bootstrap) button or an image button. If this is a correct pattern\behavior we should probably find a way to put this in a reusable component, the last time I looked around mixins are the cools for this stuff.

@mbarto
Copy link
Contributor

mbarto commented Nov 3, 2015

Unfortunately, mixins are deprecated, since they cannot be used with ES6 classes.
In general, my preferred approach is to split UI from behaviour, so that we have agnostic UI controls (buttons, image buttons, etc.), triggering desired behaviour by configuration (onClick). I don't like too much the current approach of having the UI and behaviour in the same place, or at least I don't like having this at the framework level. It's ok for now, but I will refactor it some day :)

Remove div around the image in ImageButton component
@nmco nmco force-pushed the image-button-support branch from 29a69a3 to a50ec59 Compare November 3, 2015 08:54
@nmco
Copy link
Author

nmco commented Nov 3, 2015

Good point, I was looking at the problem in a wrong way. Behavior and UI in the same place is not a good idea. Thank you.

mbarto added a commit that referenced this pull request Nov 3, 2015
Add support for ImageButton in ZoomToMaxExtentButton component
@mbarto mbarto merged commit 7164296 into geosolutions-it:master Nov 3, 2015
@mbarto mbarto removed the ready label Nov 3, 2015
@nmco nmco deleted the image-button-support branch November 3, 2015 09:01
ale-cristofori pushed a commit to ale-cristofori/MapStore2 that referenced this pull request Nov 25, 2024
…ons-it#274 remove widget (geosolutions-it#275)

* Fix geosolutions-it#271 AutoResourceUpdate implementation

* Update context as well and others

* updating epics logic for ignoring execution based on options

* Update pr with changes to plugin config

* update revision including fix for geosolutions-it#10622
ale-cristofori pushed a commit to ale-cristofori/MapStore2 that referenced this pull request Nov 25, 2024
…not trigger http requests (geosolutions-it#278)

* Update of epics to correctly update urls and not trigger http requests

* optimize logic to migrate
ale-cristofori pushed a commit to ale-cristofori/MapStore2 that referenced this pull request Nov 25, 2024
* Fix geosolutions-it#271 updating config files

* Update configs/localConfig.json

Co-authored-by: Tobia Di Pisa <[email protected]>

* Update configs/new.json

Co-authored-by: Tobia Di Pisa <[email protected]>

* update urls

* Fix url config and reordering

---------

Co-authored-by: Tobia Di Pisa <[email protected]>
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