Set favicon default url to /favicon.ico and allow to customize it #713

f-necas · 2023-12-05T08:09:48Z

The default favicon points to /assets/favicon.ico. As favicon is often provided in root, it's useful to have it there.

github-actions · 2023-12-05T08:10:57Z

Affected libs: util-app-config, feature-record, feature-router, feature-map, feature-dataviz, util-shared, feature-search, ui-elements, feature-catalog, ui-catalog, ui-search, ui-widgets, feature-editor, ui-inputs,
Affected apps: datafeeder, datahub, metadata-editor, demo, webcomponents, search, map-viewer, metadata-converter,

🚀 Build and deploy storybook and demo on GitHub Pages
📦 Build and push affected docker images

coveralls · 2023-12-05T08:12:37Z

coverage: 82.622% (-3.8%) from 86.396%
when pulling 2e5e813 on favicon
into 173df65 on main.

fgravin

Could you give more context ?
Who can't put the favicon in the assets ?
Note that it will break all actual configs.

jahow · 2023-12-06T10:26:12Z

This is unfortunately susceptible to break all existing favicons. I would rather look for a way to customize the favicon path, for instance on the docker image startup by rewriting parts of the HTML, like we do for adding preload links here:

geonetwork-ui/tools/docker/docker-entrypoint.sh

Lines 38 to 55 in f46b998

    
           # check whether the $CUSTOM_ASSETS_PATH directory is present and not empty 
        
           if [ -d "${CUSTOM_ASSETS_PATH}" ] && [ "$(ls -A ${CUSTOM_ASSETS_PATH})" ]; then 
        
             files_count=$(find ${CUSTOM_ASSETS_PATH} -type f | wc -l) 
        
             # copy assets right away 
        
             echo "[INFO] Copying ${files_count} custom assets found in ${CUSTOM_ASSETS_PATH}..." 
        
             cp ${CUSTOM_ASSETS_PATH}. -r ${APP_FILES_PATH}${ASSETS_PATH} 
        
             # add a preload link for each asset that is an image 
        
             cd ${CUSTOM_ASSETS_PATH} 
        
             images=$(find . -type f -a \( -iname "*.png" -o  -iname "*.svg" -o -iname "*.webp" -o -iname "*.jpg" -o -iname "*.jpeg" \)) 
        
             for image in ${images} 
        
             do 
        
               echo "[INFO] Adding preload link for ${image}..." 
        
               sed -i "s@<!--%PRELOAD_LINKS%-->@<!--%PRELOAD_LINKS%-->\n<link rel=\"preload\" href=\"assets/${image}\" as=\"image\" importance=\"high\" />@" \ 
        
                 ${APP_FILES_PATH}index.html 
        
             done 
        
           else 
        
             echo "[INFO] No custom assets found at ${CUSTOM_ASSETS_PATH}" 
        
           fi

Outside of a docker usage, I feel like having a favicon point to the correct place should be easier.

f-necas · 2024-01-03T13:09:10Z

@jahow Is it ok, if we remove it like this ?

jahow · 2024-01-08T11:57:31Z

I'm sorry for all the back and forth on this issue, but I think we shouldn't merge it like that.

The way I see it currently, we should:

remove the favicon like in this PR
add an option in the config file to point to a place for a custom favicon if none is available at /favicon.ico; this is e.g. the case for www.geocat.ch, which gives a 404 for https://www.geocat.ch/favicon.ico

Otherwise we're losing functionality

Revert "feat: allowing larger default integration for datahub's favicon and ME" This reverts commit 4f960614417ab2ba96f84ea482d107d88fe03f7a. feat: remove favicon in datafeeder and datahub

f-necas · 2024-01-08T13:20:18Z

@jahow I've added the possibility to implement it from toml file.

jahow

This looks better now, thanks! I've left some comments, please wait for 2.1 to be out before merging this.

I added a "breaking change" label so that we can remember that this new setting will be mandatory for people that don't have a favicon at their website's root. Please also update the PR title accordingly, so that it makes sense when it shows up in the changelog 🙂 thanks!

jahow · 2024-01-08T15:51:17Z

apps/datahub/src/app/app.component.ts

+  private setFavicon(faviconPath: string): void {
+    const link =
+      document.querySelector("link[rel*='icon']") ||
+      document.createElement('link')
+    link['type'] = 'image/x-icon'
+    link['rel'] = 'icon'
+    link['href'] = faviconPath
+    document.getElementsByTagName('head')[0].appendChild(link)
+  }
+}


IMO this should go in https://github.com/geonetwork/geonetwork-ui/blob/739b2fa13b3eba05abf4408e2a10be38676d578c/libs/util/shared/src/lib/services/theme.service.ts

jahow · 2024-01-08T15:51:48Z

conf/default.toml

@@ -34,6 +34,9 @@ proxy_path = ""
 # More information about the translation can be found in the docs (https://geonetwork.github.io/geonetwork-ui/main/docs/reference/i18n.html)
 # languages = ['en', 'fr', 'de']

+# Use it to set custom location for favicon


Suggested change

# Use it to set custom location for favicon

# Use it to set custom location for the favicon; by default, the path `/favicon.ico` will be used

Also I think this setting should go in the "theme" section

jahow · 2024-01-08T15:56:27Z

apps/datahub/src/app/app.component.ts

+    const link =
+      document.querySelector("link[rel*='icon']") ||
+      document.createElement('link')
+    link['type'] = 'image/x-icon'


this will make the favicon fail if it is in another format, e.g. PNG; to remove

jahow

All good thanks!

f-necas requested a review from jahow December 5, 2023 08:26

fgravin reviewed Dec 5, 2023

View reviewed changes

f-necas mentioned this pull request Dec 11, 2023

All applications should use the global favicon georchestra/georchestra#4087

Closed

f-necas force-pushed the favicon branch from 0df1542 to 423cf03 Compare December 21, 2023 09:27

f-necas requested a review from fgravin January 3, 2024 13:09

feat: allowing larger default integration for datahub's favicon and ME

61ed24d

Revert "feat: allowing larger default integration for datahub's favicon and ME" This reverts commit 4f960614417ab2ba96f84ea482d107d88fe03f7a. feat: remove favicon in datafeeder and datahub

f-necas force-pushed the favicon branch from 423cf03 to 61ed24d Compare January 8, 2024 12:19

feat: set a customisable favicon for datahub

739b2fa

jahow added this to the 2.2.0 milestone Jan 8, 2024

jahow added the breaking change Requires an adaptation by the user to keep feature parity label Jan 8, 2024

jahow reviewed Jan 8, 2024

View reviewed changes

feat: move icon to theme section

2e5e813

f-necas requested a review from jahow January 8, 2024 16:43

f-necas changed the title ~~Allowing larger default integration for datahub's favicon and ME~~ Set favicon default url to /favicon.ico and make it customizable Jan 8, 2024

f-necas changed the title ~~Set favicon default url to /favicon.ico and make it customizable~~ Set favicon default url to /favicon.ico and allow to customize it Jan 8, 2024

jahow approved these changes Jan 8, 2024

View reviewed changes

f-necas merged commit ed6fbd6 into main Jan 9, 2024
7 checks passed

f-necas deleted the favicon branch January 9, 2024 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set favicon default url to /favicon.ico and allow to customize it #713

Set favicon default url to /favicon.ico and allow to customize it #713

f-necas commented Dec 5, 2023

github-actions bot commented Dec 5, 2023 •

edited

Loading

coveralls commented Dec 5, 2023 •

edited

Loading

fgravin left a comment

jahow commented Dec 6, 2023 •

edited

Loading

f-necas commented Jan 3, 2024

jahow commented Jan 8, 2024

f-necas commented Jan 8, 2024

jahow left a comment

jahow Jan 8, 2024

jahow Jan 8, 2024

jahow Jan 8, 2024

jahow Jan 8, 2024

jahow left a comment

	# Use it to set custom location for favicon
	# Use it to set custom location for the favicon; by default, the path `/favicon.ico` will be used

Set favicon default url to /favicon.ico and allow to customize it #713

Set favicon default url to /favicon.ico and allow to customize it #713

Conversation

f-necas commented Dec 5, 2023

github-actions bot commented Dec 5, 2023 • edited Loading

coveralls commented Dec 5, 2023 • edited Loading

fgravin left a comment

Choose a reason for hiding this comment

jahow commented Dec 6, 2023 • edited Loading

f-necas commented Jan 3, 2024

jahow commented Jan 8, 2024

f-necas commented Jan 8, 2024

jahow left a comment

Choose a reason for hiding this comment

jahow Jan 8, 2024

Choose a reason for hiding this comment

jahow Jan 8, 2024

Choose a reason for hiding this comment

jahow Jan 8, 2024

Choose a reason for hiding this comment

jahow Jan 8, 2024

Choose a reason for hiding this comment

jahow left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2023 •

edited

Loading

coveralls commented Dec 5, 2023 •

edited

Loading

jahow commented Dec 6, 2023 •

edited

Loading