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

Fix #9653 embedded icons are correctly loaded #9660

Merged
merged 6 commits into from
Nov 27, 2023

Conversation

MV88
Copy link
Contributor

@MV88 MV88 commented Oct 25, 2023

Description

It seems that simply adding the css to the html file fixed the issue

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

Issue

What is the current behavior?

Fix #9653

What is the new behavior?

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

Other useful information

@MV88 MV88 added bug annotations related to the annotation tool C027-COMUNE_FI-2023-SUPPORT labels Oct 25, 2023
@MV88 MV88 added this to the 2024.01.00 milestone Oct 25, 2023
@MV88 MV88 self-assigned this Oct 25, 2023
@tdipisa tdipisa removed the request for review from offtherailz October 25, 2023 15:13
Copy link
Contributor

@allyoucanmap allyoucanmap left a comment

Choose a reason for hiding this comment

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

Instead to update all project with the font-awesome links in all template I would like to propose a different approach:

  • remove all the from all the html files
  • create a loadFontAwesome promise that check if the font-awesome css is in the page if not add it an wait the load or error
  • place the loadFontAwesome just before the rendering of markers in the drawIcons promise of StyleParserUtils.js

Promise to load the link stylesheet:

const loadFontAwesome = () => {
    return new Promise((resolve) => {
        const fontAwesomeHref = 'https://cdnjs.cloudflare.com/ajax/libs/font-awesome/4.7.0/css/font-awesome.min.css';
        if (!document.querySelector(`link[href='${fontAwesomeHref}']`)) {
            const fontAwesome = document.createElement('link');
            fontAwesome.setAttribute('rel', 'stylesheet');
            fontAwesome.setAttribute('href', fontAwesomeHref);
            document.head.appendChild(fontAwesome);
            fontAwesome.onload = () => {
                resolve();
            };
            fontAwesome.onerror = () => {
                resolve();
            };
        } else {
            resolve();
        }
    });
};

the we could use it inside drawIcons

export const drawIcons = (geoStylerStyle, options) => {
   ...
   return loadFontAwesome().then(() => new Promise((resolve) => {
   ...

I did a quick test and it seems to work but I would better to double check if we decide to give it a try. The advantage of this approach are:

  • font awesome will be loaded only when we are using a vector layer
  • no need to add the link to existing custom project in the html files

@MV88 MV88 requested a review from allyoucanmap November 27, 2023 10:27
@MV88
Copy link
Contributor Author

MV88 commented Nov 27, 2023

@allyoucanmap please review it again

@allyoucanmap allyoucanmap merged commit 69b29cf into geosolutions-it:master Nov 27, 2023
4 checks passed
@allyoucanmap
Copy link
Contributor

@ElenaGallo please test this fix on DEV, thanks

@ElenaGallo
Copy link
Contributor

@MV88 I noticed that:

  • the first time a point is added the icon is not visible
1.mp4
  • now, in the embedded map (2D and 3D), the marker is too large
2.mp4
  • on share map via a direct link (2D and 3D), the icon is not visible
3.mp4
  • on permalink (2D and 3D), the icon is not visible
4.mp4

@MV88
Copy link
Contributor Author

MV88 commented Nov 29, 2023

@ElenaGallo please test it in DEV

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Annotations - Icons issue in embedded maps
3 participants