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 background layer selector as 'drop-down' button in desktop apps #1299

Merged
merged 1 commit into from
May 30, 2016

Conversation

adube
Copy link
Contributor

@adube adube commented May 26, 2016

This PR is the replacement of #1291. It adds the gmf-backgroundlayerselector inside both desktop applications using contextual menu customized to look like a button.

Original commit: pgiraud@9f6b0a1

Todo

  • review

Live demos

@adube
Copy link
Contributor Author

adube commented May 26, 2016

Ready for review.

<button
class="btn btn-default"
data-toggle="dropdown">
<img src="https://cloud.githubusercontent.com/assets/319774/13696817/1cc9a7ee-e769-11e5-85c8-a1ea5ff21a5d.png" alt="" />
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to add the image in the repository?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please, see with @pgiraud or @fredj . I don't know the answer to that question.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should definitely use a local image. It was OK to use a URL only in a prototype context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood. I'm on it.

@adube
Copy link
Contributor Author

adube commented May 26, 2016

Image isn't shown on github.io.

@pgiraud
Copy link
Contributor

pgiraud commented May 26, 2016

Image isn't shown on github.io.

It doesn't show in built mode I think. You have to use the same strategy as for the geomapfish logo.
https://github.com/camptocamp/ngeo/blob/master/Makefile#L465-L468

@sbrunner
Copy link
Member

@adube you should do some thing like that #1291 (comment), and what about put the image in contribs/gmf/images?

@adube
Copy link
Contributor Author

adube commented May 26, 2016

O~kay! All raised issues have been fixed. Let me know if you need me to fix anything else.

@adube
Copy link
Contributor Author

adube commented May 26, 2016

@sbrunner Yeah, that's what has just been done. The image was put in the image directories of the applications.

@sbrunner
Copy link
Member

OK :-)

@pgiraud
Copy link
Contributor

pgiraud commented May 27, 2016

This looks good to me. Can you please squash the commits?
The last thing that I dislike are the margins in the selector itself but it should be addressed in an other issue. I'll open one.

@adube
Copy link
Contributor Author

adube commented May 27, 2016

"Squashing squashing..."

@adube
Copy link
Contributor Author

adube commented May 27, 2016

Quick unrelated question: I was told a few weeks ago that it wasn't required anymore to squash commits into once since it's now possible to automatically do so when merging on Github. I used to always squash the commits, but was told that I wasn't forced to do so anymore. Is that still the case ? I'm okay with doing one or the other, but let's agree on just one way to go.

I think I'd slightly prefer "not squashing" them manually. This makes it easier for reviewers. If one asks: "please, fix this" then he or she can immediately see in the next commit that it has been fixed without having to look at the whole code again.

What do you think ?

@adube
Copy link
Contributor Author

adube commented May 30, 2016

Is this ready to be merged ?

@fredj
Copy link
Member

fredj commented May 30, 2016

ready, I will merge when travis is green

@fredj fredj merged commit 76be506 into camptocamp:master May 30, 2016
@adube adube deleted the bls-in-desktop-apps branch May 30, 2016 13:25
@sbrunner sbrunner added this to the Older milestone Aug 23, 2019
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.

None yet

4 participants