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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions web/client/actions/geostory.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,12 +166,14 @@ export const toggleSettingsPanel = (withSave = false) => ({ type: TOGGLE_SETTING
* @param {string} path the path of the element to modify. It can contain path like this `sections[{"id": "abc"}].contents[{"id": "def"}]` to resolve the predicate between brackets.
* @param {object} element the object to update
* @param {string|object} [mode="replace"] "merge" or "replace", if "merge", the object passed as element will be merged with the original one (if present and if it is an object)
* @param {object} "uniquebykey" or "undefined", if "uniquebykey" via merge mode, the key will be checked to avoid duplication of fonts
*/
export const update = (path, element, mode = "replace") => ({
export const update = (path, element, mode = "replace", options) => ({
type: UPDATE,
path,
element,
mode
mode,
options
});
/**
* updates the current page with current value of sectionId (future can be extended adding other info about current content).
Expand Down
3 changes: 2 additions & 1 deletion web/client/plugins/GeoStory.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,8 @@ const GeoStory = ({
}, []);

useEffect(() => {
onUpdate("settings.theme.fontFamilies", fontFamilies, "merge");
// Appended options in actions for key handling to avoid fonts duplication
onUpdate("settings.theme.fontFamilies", fontFamilies, "merge", {uniqueByKey: "family"});
// we need to store settings for media editor
// so we could use them later when we open the media editor plugin
if (mediaEditorSettings) {
Expand Down
6 changes: 3 additions & 3 deletions web/client/plugins/__tests__/GeoStory-test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,13 @@ describe('GeoStory Plugin', () => {
ReactDOM.render(<Plugin webFont={{load: () => {}}} />, document.getElementById("container"));
expect(document.getElementsByClassName('ms-geostory').length).toBe(1);
});
it('Dispatches update action and sets fontFamilies', () => {
it('Dispatches update action, sets fontFamilies in merge mode', () => {
const { Plugin, actions, store } = getPluginForTest(GeoStory, stateMocker({geostory}));
const fontFamilies = [{family: "test", src: "test"}];
ReactDOM.render(<Plugin webFont={{load: () => {}}} fontFamilies={fontFamilies} />, document.getElementById("container"));
ReactDOM.render(<Plugin webFont={{load: () => {}}} mode="merge" fontFamilies={fontFamilies} />, document.getElementById("container"));

// expect to have dispatched update action once from useEffect(callback, [])
expect(actions.length).toEqual(1);
expect(actions.length).toBe(1);
expect(store.getState().geostory.currentStory.settings.theme.fontFamilies).toEqual(fontFamilies);
});
it('should store the media editor setting with onUpdateMediaEditorSetting', () => {
Expand Down
16 changes: 16 additions & 0 deletions web/client/reducers/__tests__/geostory-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,22 @@ describe('geostory reducer', () => {
});
describe('update contents', () => {
const STATE_STORY = geostory(undefined, setCurrentStory(TEST_STORY));
it('should handle UPDATE action with fontFamilies and mode merge', () => {
const initialState = {
currentStory: {
fontFamilies: ['Arial', 'Helvetica']
}
};
const action = {
type: 'UPDATE',
path: 'fontFamilies',
mode: 'merge',
element: ['Times New Roman', 'Arial', 'Verdana'],
options: { uniqueByKey: 'id' }
};
const newState = geostory(initialState, action);
expect(newState).toBe(initialState);
});
it('update with index path', () => {
const TEST_CONTENT = "<h1>UNIT TEST CONTENT</h1>";
const pathToContentHtml = `sections[0].contents[0].html`;
Expand Down
5 changes: 3 additions & 2 deletions web/client/reducers/geostory.js
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,7 @@ export default (state = INITIAL_STATE, action) => {
)(state);
}
case UPDATE: {
const { path: rawPath, mode } = action;
const { path: rawPath, mode, options } = action;
let { element: newElement } = action;
const path = getEffectivePath(`currentStory.${rawPath}`, state);
const oldElement = get(state, path);
Expand All @@ -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

}
return set(path, newElement, state);
}
Expand Down
Loading