8000 Support customization of link text by yanis-sadeg · Pull Request #287 · domoritz/leaflet-locatecontrol · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Support customization of link text #287

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Jun 25, 2021
Merged

Support customization of link text #287

merged 11 commits into from
Jun 25, 2021

Conversation

yanis-sadeg
Copy link
Contributor
@yanis-sadeg yanis-sadeg commented Jun 24, 2021 •
edited
Loading

Description

Added functionality to add text to the location button.
It is activated with the "text" option and does not allow HTML characters.

Tested on Firefox, Chrome, Safari. ✔️

Example

lc = L.control.locate({
    strings: {
        title: "Show me where I am, yo!",
        text: "Locate me"
    }
}).addTo(map);

Screenshot

Leaflet locate link text

oussama sadeg and others added 3 commits June 24, 2021 20:30
Support customization of link text
bb98c4d
@yanis-sadeg
Update README.md
a00264c 
The link of the demo now redirects correctly in https.
@yanis-sadeg
Update L.Control.Locate.js
b0f8788 
Update of the formatting.
domoritz
domoritz approved these changes Jun 24, 2021
View reviewed changes
Copy link
Owner
@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. Can you add a screenshot to the description?

src/L.Control.Locate.js Outdated

if(options.strings.text !== undefined) {
var text = L.DomUtil.create(options.textElementTag, 'leaflet-locate-text', link);
text.textContent = '\xa0' + options.strings.text;
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you need to add \xa0?

Copy link
Contributor Author
@yanis-sadeg yanis-sadeg Jun 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why do you need to add \xa0?

To make a space between the icon and the text, otherwise they are stuck together and it's ugly. I had the choice to do it by css or like this, i choosen this method. But thinking about, maybe I should make the space by css on the icon with a padding right, like this if the developer choose to use only the text without the icon there is no space for nothing. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

Yep, let's do it in CSS.

src/L.Control.Locate.js Outdated
if(options.strings.text !== undefined) {
var text = L.DomUtil.create(options.textElementTag, 'leaflet-locate-text', link);
text.textContent = '\xa0' + options.strings.text;
link.classList.add('leaflet-locate-text-active');
Copy link
Owner

Choose a reason for hiding this comment

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

indentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indentation?

Yes... In my side it's correctly indented, I don't know why the indentation on this preview is wrong. I will check about it for the next push.

Copy link
Owner

Choose a reason for hiding this comment

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

Make sure you use spaces.

src/L.Control.Locate.scss Outdated
@@ -15,6 +15,15 @@
}
}

.leaflet-touch .leaflet-bar .leaflet-locate-text-active {
Copy link
Owner

Choose a reason for hiding this comment

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

What does this have to do with the proposed change?

Copy link
Contributor Author
@yanis-sadeg yanis-sadeg Jun 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This allows to modify the css only if the text option is used, it will not impact the button if the option is not used. Also, I don't use the .leaflet-control-locate because otherwise I would have to use !important on the width property.

Modifications have been made on the css if the text option is displayed because of the default properties of leaflet which made it impossible to display the text correctly.

@yanis-sadeg
Copy link
Contributor Author
yanis-sadeg commented Jun 25, 2021 •
edited
Loading

Thanks for the pull request. Can you add a screenshot to the description?

Sure, I will on the next push.

Edite: The requested changes have been made.

yanis-sadeg and others added 6 commits June 25, 2021 09:32
@yanis-sadeg @domoritz
Update README.md
948fc5f 
The way to start the project has been changed.

Co-authored-by: Dominik Moritz <domoritz@gmail.com>
@yanis-sadeg @domoritz
Update src/L.Control.Locate.js
c0db263 
formatting changed

Co-authored-by: Dominik Moritz <domoritz@gmail.com>
Added screenshot-text.png
560eb59 
README.md updated: The new screenshot-text.png has been added

A padding right has been added on the icon's selector (works only if the text option is activated to avoid an extra padding)
Icon padding works only if the text and icon are used together
5f19ce9
Unused variable removed
152264a
indentation
f4c177b
@domoritz
Copy link
Owner

I meant a screenshot in the pull request description not the code. I just want some documentation for the feature here. Sorry for being ambiguous.

@yanis-sadeg
Copy link
Contributor Author
yanis-sadeg commented Jun 25, 2021 •
edited
Loading

I meant a screenshot in the pull request description not the code. I just want some documentation for the feature here. Sorry for being ambiguous.

My bad, I had misunderstood. Done 👌

domoritz
domoritz reviewed Jun 25, 2021
View reviewed changes
@domoritz domoritz merged commit 3bbdf2c into domoritz:gh-pages Jun 25, 2021
@domoritz
Copy link
Owner

Thank you for the addition!

@domoritz domoritz mentioned th 6A77 is pull request Jun 26, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domoritz domoritz domoritz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yanis-sadeg @domoritz
0