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 #10786 Review of the home description image #10823

Merged
merged 7 commits into from
Feb 24, 2025

Conversation

allyoucanmap
Copy link
Contributor

Description

This PR introduces following changes:

  • review HomeDescription plugin structure and configuration and remove old HomeDescription from product folder
  • include images for hero, favicon and logo
  • align the position of assets folder to downstream project

image

Please check if the PR fulfills these requirements

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

  • Minor changes to existing features

Issue

What is the current behavior?

#10786

What is the new behavior?

Review of HomeDescription plugin

Breaking change

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

  • Yes, and I documented them in migration notes

Other useful information

@allyoucanmap allyoucanmap added this to the 2025.01.00 milestone Feb 19, 2025
@allyoucanmap allyoucanmap self-assigned this Feb 19, 2025
@allyoucanmap allyoucanmap linked an issue Feb 19, 2025 that may be closed by this pull request
1 task
@allyoucanmap
Copy link
Contributor Author

@tdipisa this PR contains changes to update the location of assets folder to make it more consistent with downstream project.
The idea is to have the same location for files, in particular images:

|__ assets/
    |__ img/
         |__ favicon.png
         |__ hero.jpg
         |__ logo.png

If we keep the path of assets consistent we could reduce additional configuration to register location of images in localConfig.json and instead we could add a image with the same name to replace both hero, favicon or logo.
There are also other assets that could benefit from this change such as custom annotations symbols but they have not been included in this PR.
If the work in this PR will be accepted I think we should open an additional issue to keep track of a possible improvement related to the assets folder

@tdipisa tdipisa requested a review from offtherailz February 19, 2025 14:20
Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

Here I should avoid to introduce the assets folder for this task.
I undertand that this could help for future projects to customize by editing simply by editing the assets folder, instead of editing all the configurations.

Anyway it looks to require:

  • duplications (between project and template).
  • not clear migration. Old projects may not find the assets folder, (not mentioned in migration guidelines).
  • maintainance (assets folder may diverge between projects).

Having the possibility to publish assets on the product (and maybe override via data dir) is a good feature, but premature. It is also somehow provided by extensions, we can evaluate it in a separate way.

I'd suggest a couple of different approaches that are more robust:
For all assets (logo, hero):

  • Is is possible to put the images in the theme, so the overriding of branding is delegated to the theme, and properly document how to migrate?
  • Import the default images as usual (with webpack). For logo, use a global variable in localConfig.json (to be shared across congurations).

For favicon, it is managed by HTMLWebpackPlugin, so maybe we can use that property, to allow customizations.

I discourage to use assets folder, but if this will be the case it have to be:

  • Properly documented
  • Need to document also migration

Comment on lines 51 to 54
},
'/assets': {
target: "http://localhost:8081",
pathRewrite: {'/assets': '/product/assets'} // mapping the assets similarly to a downstream project
Copy link
Member

Choose a reason for hiding this comment

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

This will break the assets folder in dev environment of downstream project, forcing every project to create their ownl devServer just to skip this rule.
I'd suggest to use assets folder directly in the root of the project, not in product folder, removing this level of complexity.

@allyoucanmap
Copy link
Contributor Author

Here I should avoid to introduce the assets folder for this task. I undertand that this could help for future projects to customize by editing simply by editing the assets folder, instead of editing all the configurations.

Anyway it looks to require:

  • duplications (between project and template).
  • not clear migration. Old projects may not find the assets folder, (not mentioned in migration guidelines).
  • maintainance (assets folder may diverge between projects).

Having the possibility to publish assets on the product (and maybe override via data dir) is a good feature, but premature. It is also somehow provided by extensions, we can evaluate it in a separate way.

I'd suggest a couple of different approaches that are more robust: For all assets (logo, hero):

  • Is is possible to put the images in the theme, so the overriding of branding is delegated to the theme, and properly document how to migrate?
  • Import the default images as usual (with webpack). For logo, use a global variable in localConfig.json (to be shared across congurations).

For favicon, it is managed by HTMLWebpackPlugin, so maybe we can use that property, to allow customizations.

I discourage to use assets folder, but if this will be the case it have to be:

  • Properly documented
  • Need to document also migration

@offtherailz (cc @tdipisa) as agreed I removed changes related to assets location, I'll open a separate issue to discuss and fix it

@allyoucanmap
Copy link
Contributor Author

@offtherailz (cc @tdipisa) as agreed I removed changes related to assets location, I'll open a separate issue to discuss and fix it

here the new issue #10836

@allyoucanmap allyoucanmap merged commit 06be6f9 into geosolutions-it:master Feb 24, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review of the home description image
2 participants