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 duplicate fonts in Geonode MapStore Client - GeoStory Edit Mode #9686

Conversation

mumairr
Copy link
Contributor

@mumairr mumairr commented Nov 8, 2023

Description

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 GeoNode/geonode-mapstore-client#1593

What is the new behavior?

When passing custom fonts, an object with unique identifier is passed to check & restrict duplicate entries inside GeoStory plugin in edit mode

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

Issue was only reproducible on Geonode MapStore Client due to async loading of GeoStory component

Custom fonts should be added to localconfig under GeoStory -> cfg:
{
"name": "GeoStory",
"cfg": {
"fontFamilies": [
{
"family": "Arial"
},
{
"family": "Georgia"
},
{
"family": "Impact"
},
{
"family": "Tahoma"
},
{
"family": "Times New Roman"
},
{
"family": "Titillium Web",
"src": "https://fonts.googleapis.com/css2?family=Titillium+Web"
},
{
"family": "Verdana"
}
]
}
}

"family": "Times New Roman"
},
{
"family": "Titillium Web",
Copy link
Contributor

Choose a reason for hiding this comment

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

@mumairr I don't think we want to include Trillium in MapStore. Are you sure about this hardcoded list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@giohappy no we don't and we shouldn't. It was merely to support the idea of changes requested in the PR.
Should I remove this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mumairr yes please remove those font in the default configuration, just mention in the PR description to add this to properly test the fixes, thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

The font configuration is still here, please restore the localConfig

@giohappy giohappy requested a review from allyoucanmap November 9, 2023 09:28
"family": "Times New Roman"
},
{
"family": "Titillium Web",
Copy link
Contributor

Choose a reason for hiding this comment

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

@mumairr yes please remove those font in the default configuration, just mention in the PR description to add this to properly test the fixes, thanks

@@ -317,7 +317,8 @@ export default (state = INITIAL_STATE, action) => {
}

if (isArray(oldElement) && isArray(newElement) && mode === "merge") {
newElement = [ ...oldElement, ...newElement ];
// check for options to filter unique options
newElement = (options?.uniqueByKey) ? uniqBy([ ...oldElement, ...newElement ], options.uniqueByKey) : [...oldElement, ...newElement ];
Copy link
Contributor

Choose a reason for hiding this comment

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

please add also a test for the reducer, thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, working on it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing the test in the reducers but a new one in the plugin, please add a new unit test for this case in the reducer https://github.com/geosolutions-it/MapStore2/blob/master/web/client/reducers/__tests__/geostory-test.js

@mumairr
Copy link
Contributor Author

mumairr commented Nov 10, 2023

Custom fonts removed from localconfig, appended in PR description. Reducer test added for the GeoStory Plugin

"family": "Times New Roman"
},
{
"family": "Titillium Web",
Copy link
Contributor

Choose a reason for hiding this comment

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

The font configuration is still here, please restore the localConfig

@allyoucanmap
Copy link
Contributor

allyoucanmap commented Nov 21, 2023

Custom fonts removed from localconfig, appended in PR description. Reducer test added for the GeoStory Plugin

@mumairr please take a look to these comments:

#9686 (comment)

#9686 (comment)

@mumairr
Copy link
Contributor Author

mumairr commented Nov 21, 2023

Custom fonts removed from localconfig, appended in PR description. Reducer test added for the GeoStory Plugin

@mumairr please take a look to these comments:

#9686 (comment)

#9686 (comment)

On it Stefano

@mumairr
Copy link
Contributor Author

mumairr commented Nov 26, 2023

@allyoucanmap

test for reducer added, removed from geostory plugin.
Fonts removed from localconfig

@allyoucanmap allyoucanmap removed the request for review from giohappy November 27, 2023 10:53
@giohappy giohappy self-requested a review November 27, 2023 11:02
@giohappy giohappy merged commit 2967b02 into geosolutions-it:master Nov 27, 2023
4 checks passed
@allyoucanmap
Copy link
Contributor

@ElenaGallo please test in dev and let us know if we can backport on 2022.02.xx for geonode-mapstore-client, thanks

@ElenaGallo
Copy link
Contributor

Test passed, @mumairr please backport on 2022.02.xx for geonode-mapstore-client. Thanks

@mumairr mumairr mentioned this pull request Nov 27, 2023
12 tasks
@mumairr
Copy link
Contributor Author

mumairr commented Nov 27, 2023

@ElenaGallo Thanks Elena.

@allyoucanmap Stefano, backport PR is available at #9759

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.

In edit mode, custom font added to Geostory occurs twice in editor
4 participants