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

ISSUE-199 - Grouping of icons to search for an owner/co-owner #205

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

LPoin
Copy link
Contributor

@LPoin LPoin commented Dec 20, 2024

!! MERGE AFTER PR 202 FOR ISSUE-197 !!

  • Add a new icon in the toolbar for searching owner & co-owner
  • Modify positon of search owner & co-owner buttons
  • Search owner & co-owner panel unchanged
  • Modify TButton to accept custom icons

@landryb
Copy link
Member

landryb commented Dec 23, 2024

same as #203, needs plumbing to install the svg, otherwise you get a broken icon:
image

remark on the selected tool icon: it should be green not grey to hilight which one from owner/co-owner is active.. same thing for the new button color, it should be green if we're on the owner tools pane.

there's a weird behaviour, if you click on the (already selected) tool button, it .. comes back to the cadastrapp welcome pane ?

@pierrejego pierrejego self-requested a review December 23, 2024 09:00
@pierrejego pierrejego self-assigned this Dec 23, 2024
@pierrejego pierrejego added this to the v2.2.2 milestone Dec 23, 2024
@pierrejego pierrejego marked this pull request as draft December 23, 2024 09:01
@pierrejego
Copy link
Member

Not tested in integration for the moment, only in dev

@Gaetanbrl
Copy link

Gaetanbrl commented Dec 24, 2024

needs plumbing to install the svg

We find build patch to includes SVG files in build process. I dunno if they finally works well....

const fileLoader = {
test: /\.(ttf|eot|svg)(\?v=[0-9].[0-9].[0-9])?$/,
use: [{
loader: 'file-loader',
options: {
name: "[name].[ext]"
}
}]
};

For this pull, we can set image source from svg data string.

Copy link

@Gaetanbrl Gaetanbrl left a comment

Choose a reason for hiding this comment

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

J'opterai pour une autre manière de gérer l'icône avec une image en n'utilsant pas uniquement la clé glyph (pour moi réservée dans MapStore à une valeur de glyphicon).

js/extension/plugins/cadastrapp/toolbar/SearchTools.jsx Outdated Show resolved Hide resolved
js/extension/plugins/cadastrapp/toolbar/SearchTools.jsx Outdated Show resolved Hide resolved
js/extension/plugins/cadastrapp/toolbar/TButton.jsx Outdated Show resolved Hide resolved
js/extension/plugins/cadastrapp/toolbar/SearchTools.jsx Outdated Show resolved Hide resolved
js/extension/plugins/cadastrapp/SearchSection.jsx Outdated Show resolved Hide resolved
@LPoin
Copy link
Contributor Author

LPoin commented Dec 24, 2024

Tested in integration.
Mapstore 2023.02

  • SVG fixed
  • "if you click on the (already selected) tool button, it .. comes back to the cadastrapp welcome pane", it's the current behaviour, if you click on en tool already selected, it close the panel and return to welcome panel.

@landryb
Copy link
Member

landryb commented Dec 26, 2024

For this pull, we can set image source from svg data string.

im not a fan of this solution since it makes it very hard to (eventually) override the icon.. we should be able to use svgs in extensions, im surprised there's no working example elsewhere ?

i dunno if its my local build, but it seems this PR/ranch includes #202 since i get to test both when building this branch... which is perfect from a testing POV, not sure from a project mgmt POV :)

the ppl search icon renders okay here, there's just a small inconsistency in title positioning wrt icons in the pane (above/below):

image
vs
image

that makes the display weird when you switch from one to the other.

  • "if you click on the (already selected) tool button, it .. comes back to the cadastrapp welcome pane", it's the current behaviour, if you click on en tool already selected, it close the panel and return to welcome panel.

ah, maybe that's the current behaviour, right. But ... from an UX pov, isn't it... super confusing ? :)

@Gaetanbrl
Copy link

im surprised there's no working example elsewhere ?

Same for me. But SVG's build config seems only apply in mapstore2-cadatrapp repo.

This works well in dev mode but not after extension build process (due to different build config / process).

it makes it very hard to (eventually) override the icon

You mean if you want to override via the extensions directory (datadir) ?

@Gaetanbrl
Copy link

Gaetanbrl commented Dec 27, 2024

(due to different build config / process).

I suppose webpack config on simple npm:start use mapstore2 config with a SVG management :

...and extension build (to create zip) use a different config :

Note that npm ext:start should runs mapstore + extension locally in a state close to production env. But this not works correctly with mapstore2-cadastrapp...

@landryb
Copy link
Member

landryb commented Dec 27, 2024

You mean if you want to override via the extensions directory (datadir) ?

yes, since here the extension is unpacked in the georchestra datadir, having the icon as an asset/svg file makes it easy to override/historize in git.

...and extension build (to create zip) use a different config :

* https://github.com/georchestra/mapstore2-cadastrapp/blob/master/package.json#L113

now that's a good catch. with that local diff on top of this branch, and the owners.svg file in the toplevel assets subdir:

[27/12 10:15] [email protected]:/data/src/georchestra/mapstore2-cadastrapp $git diff build/extension/prod-webpack.config.js 
diff --git a/build/extension/prod-webpack.config.js b/build/extension/prod-webpack.config.js
index bf61990..73431f8 100644
--- a/build/extension/prod-webpack.config.js
+++ b/build/extension/prod-webpack.config.js
@@ -12,6 +12,7 @@ const plugins = [
     new CopyPlugin([
         { from: path.resolve(__dirname, "..", "..", "assets", "translations"), to: "translations" },
+        { from: path.resolve(__dirname, "..", "..", "assets", "owners.svg"), to: "owners.svg" },
         { from: path.resolve(__dirname, "..", "..", "assets", "index.json"), to: "index.json" }

     ]),
     new ZipPlugin({
         filename: `${name}.zip`,

and building the extension:

[27/12 10:15] [email protected]:/data/src/georchestra/mapstore2-cadastrapp $npm install && npm run ext:build

the resulting zip contains the svg file at the toplevel dir:

[27/12 10:22] [email protected]:/data/src/georchestra/mapstore2-cadastrapp $unzip -l dist/Cadastrapp.zip |grep svg
     5551  2024-12-27 10:21   spritesheet.svg
   444379  2024-12-27 10:21   fontawesome-webfont.svg
      570  2024-12-27 10:21   symbolMissing.svg
     4322  2024-12-27 10:21   owners.svg

that could be used/reached from the extension code probably the same way how translations/index.json is imported/used. Or find the right magic so that the svg is copied under assets/img within the zip ?

btw i think the same thing/construct should be done to install the svg in #204 for the extension icon. instead of inlining the svg in a js file....

@Gaetanbrl
Copy link

Gaetanbrl commented Dec 27, 2024

the resulting zip contains the svg file at the toplevel dir:

Indeed, we have this behvior with SVG but it's a real ngithmare :

  • in dev mode -> All works fine if we locate SVG file in js/extension/assets folder
  • we start build
  • SVG is at the top level and component (.jsx) -> No image exists under this path

So, to avoid to disrupt webpack process and ease next / potential upgrade (if process changes) i'd suggest to use SVG string (juste copy / past SVG to constant file seems easy).

Or find the right magic so that the svg is copied under assets/img within the zip ?

That's the question we discussed this morning with @LPoin ...

@LPoin
Copy link
Contributor Author

LPoin commented Jan 3, 2025

About our SVG issue :

const fileLoader = {
    test: /\.(ttf|eot|svg)(\?v=[0-9].[0-9].[0-9])?$/,
    use: [{
        loader: 'file-loader',
        options: {
            name: "[name].[ext]"
        }
    }]
};

It treats svg as modules and places them in the defined output, in this case the root.

Tested solutions :

  • Add our custom icon in glyphIcons : need external app to include our custom svg, but depprecated (bootstrap 3 not maintained).
  • Add external loader : don't work neither.
  • Modify fileLoader in prod-webpack.config.js in build/extension (file used for npm run ext:build) to put svg in "asset/img/[name].[ext]". The path is not recognised once the build is complete.

Current test :

  • Modify prod-webpack.config.js to create new paths according to relative path of SVG file imported.

@LPoin
Copy link
Contributor Author

LPoin commented Jan 7, 2025

I try to modify prod-webpack.config.js (used in ext:build run) to fiw svg issue.
Webpack is now in version 5.54.0 for 3 months and many elements don't works as before.
I modify fileLoader to

const fileLoader = {
    test: /\.(ttf|eot|svg)(\?v=[0-9].[0-9].[0-9])?$/,
    type: 'asset/resource', // file loader for v5 webpack
    generator: {
        filename: 'assets/[name][ext]',
        publicPath: '/',
    },
};

as it work in V5.
I have to manage polyfills as webpack 5 removed automatics polyfills for node.js.
And I add alias to manage import with alias in files and supported extensions

alias: {
            "@mapstore/patcher": path.resolve(__dirname, '..', '..', "node_modules", "@mapstore", "patcher"),
            "@mapstore": path.resolve(__dirname, '..', '..', "MapStore2", "web", "client"),
            "@js": path.resolve(__dirname, '..', '..', "js"),
        },
extensions: ['.js', '.jsx', '.json']

With all these changes, build is fixed, svg are stored in the right place but I still have an error
image

It might need modifications elsewhere.
I'll create an issue on mapStore-Extension linked to this one.

@LPoin
Copy link
Contributor Author

LPoin commented Jan 8, 2025

An issue have been created in mapstore-extension here to solve the svg problem.
We could use the current solution in this PR and in PR-204 and I create a new issue to solve the problem after mapstore-extension solution.

id="svg5"
inkscape:version="1.1 (c68e22c387, 2021-05-23)"
sodipodi:docname="interface__cadastrApp.svg"
inkscape:export-filename="C:\Users\Agathe\Desktop\Cadastrapp\Refonte_interface\Cadastrapp__interfaceSearchProp.svg"
Copy link
Member

Choose a reason for hiding this comment

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

can it be removed from svg ?

@pierrejego
Copy link
Member

Conflict need to be merge

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

Successfully merging this pull request may close these issues.

4 participants