-
Notifications
You must be signed in to change notification settings - Fork 302
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
Fetching of Style.pattern images #2135
Conversation
a7b9c74
to
d69d5c2
Compare
1c3f7cc
to
1e46681
Compare
e72f45b
to
a2da04a
Compare
a2da04a
to
65055ec
Compare
65055ec
to
586f9fa
Compare
src/Core/Label.js
Outdated
style.applyToHTML(this.content, sprites); | ||
style.applyToHTML(this.content) | ||
.then(() => { | ||
this.icon = this.content.getElementsByClassName('itowns-icon')[0]; |
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 might be wrong but from my understanding we are not sure the icon element has been created in the dom when applyToHTML
is resolved (because the domelement is created in a callback called when the icon is loaded. We might need to use another await
in applyToHTML
or return a promise that resolves with the domElement
icon instead, WDYT?
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.
What if i add a new event 'loaded' that will be send when the icon is loaded ?
and then catch the event in Label.js
style.applyToHTML(this.content)
.then((icon) => {
if (icon) {
icon.addEventListener('loaded', () => {
this.icon = this.content.getElementsByClassName('itowns-icon')[0];
});
}
});
See last commit f145fb6
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.
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.
That's the idea but I think there is a simpler implementation (sorry I paste all the function but I think it's easier to understand this way). I've quickly tested it and it seems to work, can you test also + WDYT ?
async applyToHTML(domElement) {
if (arguments.length > 1) {
console.warn('Deprecated argument sprites. Sprites must be configured in style.');
}
domElement.style.padding = `${this.text.padding}px`;
domElement.style.maxWidth = `${this.text.wrap}em`;
domElement.style.color = this.text.color;
if (this.text.size > 0) {
domElement.style.fontSize = `${this.text.size}px`;
}
domElement.style.fontFamily = this.text.font.join(',');
domElement.style.textTransform = this.text.transform;
domElement.style.letterSpacing = `${this.text.spacing}em`;
domElement.style.textAlign = this.text.justify;
domElement.style['white-space'] = 'pre-line';
if (this.text.haloWidth > 0) {
domElement.style.setProperty('--text_stroke_display', 'block');
domElement.style.setProperty('--text_stroke_width', `${this.text.haloWidth}px`);
domElement.style.setProperty('--text_stroke_color', this.text.haloColor);
domElement.setAttribute('data-before', domElement.textContent);
}
if (!this.icon.source) {
return;
}
const icon = document.createElement('img');
const addIcon = () => {
const cIcon = icon.cloneNode();
cIcon.setAttribute('class', 'itowns-icon');
cIcon.width = icon.width * this.icon.size;
cIcon.height = icon.height * this.icon.size;
cIcon.style.color = this.icon.color;
cIcon.style.opacity = this.icon.opacity;
cIcon.style.position = 'absolute';
cIcon.style.top = '0';
cIcon.style.left = '0';
switch (this.icon.anchor) { // center by default
case 'left':
cIcon.style.top = `${-0.5 * cIcon.height}px`;
break;
case 'right':
cIcon.style.top = `${-0.5 * cIcon.height}px`;
cIcon.style.left = `${-cIcon.width}px`;
break;
case 'top':
cIcon.style.left = `${-0.5 * cIcon.width}px`;
break;
case 'bottom':
cIcon.style.top = `${-cIcon.height}px`;
cIcon.style.left = `${-0.5 * cIcon.width}px`;
break;
case 'bottom-left':
cIcon.style.top = `${-cIcon.height}px`;
break;
case 'bottom-right':
cIcon.style.top = `${-cIcon.height}px`;
cIcon.style.left = `${-cIcon.width}px`;
break;
case 'top-left':
break;
case 'top-right':
cIcon.style.left = `${-cIcon.width}px`;
break;
case 'center':
default:
cIcon.style.top = `${-0.5 * cIcon.height}px`;
cIcon.style.left = `${-0.5 * cIcon.width}px`;
break;
}
cIcon.style['z-index'] = -1;
domElement.appendChild(cIcon);
return cIcon;
};
const iconPromise = new Promise((resolve, reject) => {
icon.onload = () => resolve(addIcon());
icon.onerror = err => reject(err);
});
if (!this.icon.cropValues && !this.icon.color) {
icon.src = this.icon.source;
} else {
const img = await loadImage(this.icon.source);
const imgd = cropImage(img, this.icon.cropValues);
const imgdColored = replaceWhitePxl(imgd, this.icon.color, this.icon.id || this.icon.source);
canvas.getContext('2d').putImageData(imgdColored, 0, 0);
icon.src = canvas.toDataURL('image/png');
}
return iconPromise;
}
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 did so test and it seems that the test icon.complete is indeed not necessary.
I propose in the last commit some modification inspired by your code. I put out the addicon function from applyToHTML to prevent it being created many time and I fell like it makes the code a bit more readable (and from my point of view a bit more similare to applyToCanvasPolygon), WDYT ?
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.
ok 👍 can you just update the doc of applyToHTML
(especially the @returns please?)
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.
done
e8da176
to
4b996ca
Compare
f145fb6
to
606cba5
Compare
3cec734
to
5fc0047
Compare
…vision !need improvement!
5fc0047
to
8869ae0
Compare
src/Core/Label.js
Outdated
style.applyToHTML(this.content, sprites); | ||
style.applyToHTML(this.content) | ||
.then(() => { | ||
this.icon = this.content.getElementsByClassName('itowns-icon')[0]; |
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.
That's the idea but I think there is a simpler implementation (sorry I paste all the function but I think it's easier to understand this way). I've quickly tested it and it seems to work, can you test also + WDYT ?
async applyToHTML(domElement) {
if (arguments.length > 1) {
console.warn('Deprecated argument sprites. Sprites must be configured in style.');
}
domElement.style.padding = `${this.text.padding}px`;
domElement.style.maxWidth = `${this.text.wrap}em`;
domElement.style.color = this.text.color;
if (this.text.size > 0) {
domElement.style.fontSize = `${this.text.size}px`;
}
domElement.style.fontFamily = this.text.font.join(',');
domElement.style.textTransform = this.text.transform;
domElement.style.letterSpacing = `${this.text.spacing}em`;
domElement.style.textAlign = this.text.justify;
domElement.style['white-space'] = 'pre-line';
if (this.text.haloWidth > 0) {
domElement.style.setProperty('--text_stroke_display', 'block');
domElement.style.setProperty('--text_stroke_width', `${this.text.haloWidth}px`);
domElement.style.setProperty('--text_stroke_color', this.text.haloColor);
domElement.setAttribute('data-before', domElement.textContent);
}
if (!this.icon.source) {
return;
}
const icon = document.createElement('img');
const addIcon = () => {
const cIcon = icon.cloneNode();
cIcon.setAttribute('class', 'itowns-icon');
cIcon.width = icon.width * this.icon.size;
cIcon.height = icon.height * this.icon.size;
cIcon.style.color = this.icon.color;
cIcon.style.opacity = this.icon.opacity;
cIcon.style.position = 'absolute';
cIcon.style.top = '0';
cIcon.style.left = '0';
switch (this.icon.anchor) { // center by default
case 'left':
cIcon.style.top = `${-0.5 * cIcon.height}px`;
break;
case 'right':
cIcon.style.top = `${-0.5 * cIcon.height}px`;
cIcon.style.left = `${-cIcon.width}px`;
break;
case 'top':
cIcon.style.left = `${-0.5 * cIcon.width}px`;
break;
case 'bottom':
cIcon.style.top = `${-cIcon.height}px`;
cIcon.style.left = `${-0.5 * cIcon.width}px`;
break;
case 'bottom-left':
cIcon.style.top = `${-cIcon.height}px`;
break;
case 'bottom-right':
cIcon.style.top = `${-cIcon.height}px`;
cIcon.style.left = `${-cIcon.width}px`;
break;
case 'top-left':
break;
case 'top-right':
cIcon.style.left = `${-cIcon.width}px`;
break;
case 'center':
default:
cIcon.style.top = `${-0.5 * cIcon.height}px`;
cIcon.style.left = `${-0.5 * cIcon.width}px`;
break;
}
cIcon.style['z-index'] = -1;
domElement.appendChild(cIcon);
return cIcon;
};
const iconPromise = new Promise((resolve, reject) => {
icon.onload = () => resolve(addIcon());
icon.onerror = err => reject(err);
});
if (!this.icon.cropValues && !this.icon.color) {
icon.src = this.icon.source;
} else {
const img = await loadImage(this.icon.source);
const imgd = cropImage(img, this.icon.cropValues);
const imgdColored = replaceWhitePxl(imgd, this.icon.color, this.icon.id || this.icon.source);
canvas.getContext('2d').putImageData(imgdColored, 0, 0);
icon.src = canvas.toDataURL('image/png');
}
return iconPromise;
}
src/Core/Label.js
Outdated
style.applyToHTML(this.content, sprites); | ||
style.applyToHTML(this.content) | ||
.then(() => { | ||
this.icon = this.content.getElementsByClassName('itowns-icon')[0]; |
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.
ok 👍 can you just update the doc of applyToHTML
(especially the @returns please?)
fdf9691
to
f045341
Compare
Modification of style.pattern image fetching:
Based on the icon image fetching, we propose a similar approch for the style.pattern image fetching
This allow Style constructor to be cleaner.