feat: implement first step of modular #25

f-necas · 2024-08-02T12:11:17Z

This PR allows to edit header with a config file. No need to fork it anymore.

TODO :

Custom config

You can add a custom config file to the geor-header :

<geor-header active-app="datahub" config-file="/public/config.json"></geor-header>

It can contains a custom menu, config and i18n.

{
  "config": {},
  "menu": [],
  "i18n": {}
}

Config

Config can contains old tag attributes :

  "config": {
    "legacyHeader": "false",
    "legacyUrl": "/header",
    "stylesheet": "https://data.lillemetropole.fr/public/georchestra.css",
    "logoUrl": "https://data.lillemetropole.fr/public/logo-mel.jpg",
    "hideLogin": false,
    "lang": "es"
  }

i18n

Contains additional translations for the app.

landryb · 2024-12-02T14:18:00Z

oh awesome :)

landryb · 2024-12-09T08:49:02Z

there's some (testing-only?) jsonfake struct in 4ebe0c0?

f-necas · 2024-12-09T09:00:00Z

there's some (testing-only?) jsonfake struct in 4ebe0c0?

Yep, still some things to do. Small question, I'll implement a secnd active-app condition, so in the json it'll be :
active-app: datahub or active-app-url: /datahub which will respond to the condition url starts with. Could it be convenient for you ?

landryb · 2024-12-09T09:01:38Z

there's some (testing-only?) jsonfake struct in 4ebe0c0?

Yep, still some things to do. Small question, I'll implement a secnd active-app condition, so in the json it'll be : active-app: datahub or active-app-url: /datahub which will respond to the condition url starts with. Could it be convenient for you ?

if the url matching is done on the full url (eg after the mapstore /#/ router anchor) then definitely :) but i agree it might not be that simple.. and some might want to match on queryparams too.. dunno

edit: my usecase is differentiating /mapstore/ from /mapstore/#/context/cadastre, eg being able to highlight a distinct entry/icon.

f-necas · 2024-12-09T11:16:13Z

@landryb should solve everything a42e38b#diff-a0d1c9a9f2eec4ee7507a995e0254b610e5d97262285b35e057980c9ca50a159R69-R76

landryb · 2024-12-09T12:46:37Z

@landryb should solve everything a42e38b#diff-a0d1c9a9f2eec4ee7507a995e0254b610e5d97262285b35e057980c9ca50a159R69-R76

that looks clever.. ive build the PR locally and get some errors about properties not existing:

src/header.ce.vue:23:24 - error TS2339: Property 'lang' does not exist on type '{ legacyHeader: boolean; legacyUrl: string; hideLogin: boolean; style: string; }'.
src/header.ce.vue:39:24 - error TS2339: Property 'hasRole' does not exist on type 'object'.
src/header.ce.vue:42:26 - error TS2339: Property 'blockedRole' does not exist on type 'object'.
....
...

i've tried quickly with "active-app-url": "includes:/#/context/cadastre" and that seems to have the expected effect (eg the hilighted entry is the expected one on https:/.../dist/#/context/cadastre.. but also on dist/#/context/xxx ?). havent tried with other entries to see which one takes precedence over the other.

for some reason (and i have no idea if that's related to that PR) but here the headerScript value from default.properties in the datadir isnt used anymore, if set it still fetches the header.js from jsdelivr. Something that only works with the gateway ? something else i'm missing/a /config endpoint that would serve json from default.properties ?

edit nvm i think i found it, the headerscript needs to be set in mapstore's localConfig.json too.

landryb · 2024-12-09T13:38:13Z

mm.. i might be missing something, but once 'fixed' mapstore with the script param on <geor-header> in index.html, the right header.js is fetched, the config.json too, but the expected entry isnt hilighted this time:

{                                                                                                                                                                                                                                                                         
  "label": "My other service",                                                                                                                                                                                                                                            
  "url": "/geoserver/web",                                                                                                                                                                                                                                                
  "active-app-url": "includes:/#/context/cadastre"                                                                                                                                                                                                                        
},

in fact that's because window.location.pathname doesnt contain what's after the anchor (here on firefox, that's only /mapstore/). i think activeAppUrl should do the matching on window.location.href (at least for includes, if not for the others ?), if that's portable ? at least that's what i infer from https://www.samanthaming.com/tidbits/86-window-location-cheatsheet/,

landryb · 2024-12-09T15:44:01Z

just for the record: it works for the /mapstore/#/context/cadastre case with this:

--- a/src/header.ce.vue
+++ b/src/header.ce.vue
@@ -69,7 +69,7 @@ function activeAppUrl(activeAppUrl: string): boolean {
     case 'end':
       return window.location.pathname.endsWith(url)
     case 'includes':
-      return window.location.pathname.includes(url)
+      return window.location.href.includes(url)
     case 'exact':
       return window.location.pathname === url
     default:

but then it opens another can of works where multiple apps end up being 'hilighted', for which we're trying to find the best heuristic... maybe based on the full url length, eg the longest one being the 'more specific':

/mapstore/ shouldn't be hilighted as active when /mapstore/#/context/cadastre is hilighted.

landryb · 2024-12-10T08:20:25Z

retested with f1cd390 and this config section, for extra complexity:

    {
      "label": "Mapstore",
      "url": "/mapstore/#/",
      "activeApp": "mapstore"
    },
    {
      "label": "MapsApps",
      "i18n": "maps",
      "url": "/mapstore/#/home",
      "activeApp": "mapstore-home"
    },
    {
      "label": "Mapstore home",
      "url": "/mapstore/#/home",
      "activeAppUrl": "start:/mapstore/#/home"
    },
    {
      "label": "Cadastrapp",
      "url": "/mapstore/#/context/cadastre",
      "activeAppUrl": "includes:/#/context/cadastre"
    },

i think one shouldnt mix (eg it should be documented) activeApp & activeAppUrl, because:

on /mapstore/#/home the MapsApps & Mapstore home entries are hilighted
on /mapstore/#/ only the Mapstore entry is hilighted (as expected)
clicking on the cadastrapp entry loads /mapstore/#/context/cadastre, and only the Mapstore entry is hilighted (wrongly?)
reloading the page on /mapstore/#/context/cadastre, both the Mapstore and Cadastrapp entries are hilighted

there's a strange difference of behaviour whether one clicks on an entry in the header, or directly loads the url (say from an external link)

retrying with a less complex/real config:

    {
      "label": "Mapstore",
      "url": "/mapstore/#/",
      "activeAppUrl": "exact:/mapstore/#/"
    },
    {
      "label": "MapsApps",
      "url": "/mapstore/#/home",
      "activeApp": "mapstore-home"
    },
    {
      "label": "Cadastrapp",
      "url": "/mapstore/#/context/cadastre",
      "activeAppUrl": "includes:/#/context/cadastre"
    },

i have the expected behaviour if clicking on MapsApps and Cadastrapp (eg the right entry is hilighted, and only this one) but if i click on mapstore in the header, no entry is hilighted. The Mapstore entry is hilighted only if i reload the page.

we're slowly getting there :)

landryb · 2024-12-10T08:45:18Z

one think i dont understand yet is the purpose of the blockedRole config key and its interaction with the hasRole key.

with that config (taken from the PR example):

        {
          "label": "Geonetwork",
          "i18n": "catalogue",
          "url": "/geonetwork/srv/:lang3/admin.console",
          "activeApp": "geonetwork",
          "hasRole": "ROLE_SUPERUSER,ROLE_GN_EDITOR,ROLE_GN_ADMIN",
          "blockedRole": "ROLE_GN_EDITOR,ROLE_SUPERUSER"
        },
        {
          "label": "Geonetwork",
          "i18n": "catalogue",
          "url": "/geonetwork/srv/:lang3/catalog.edit#/board",
          "activeApp": "geonetwork",
          "hasRole": "ROLE_SUPERUSER,ROLE_GN_EDITOR,ROLE_GN_ADMIN",
          "blockedRole": "ROLE_GN_REVIEWER"
        },

with the testadmin user having ROLE_SUPERUSER and ROLE_GN_ADMIN (and not ROLE_GN_EDITOR/ROLE_GN_REVIEWER) i get only the second entry.

what was the intended rationale ?

users with GN_REVIEWER shouldnt access the editor board, and only has access to the GN admin ?
users with GN_EDITOR should get the editor board (and not the GN admin ) ?
an user with both GN_REVIEWER and GN_EDITOR gets none ? (thats.. confusing ?)
users with GN_ADMIN should have both the editor board and the GN admin entry ?

landryb · 2024-12-10T12:56:35Z

more testing with latest commits, with some changes to the default config:

this gives a correct behaviour (eg direct links to users/orgs tabs, and the right entry is hilighted), except that 'console' is highlighted if an admin goes to his account details page

           {
             "label": "Console",
-            "i18n": "users",
             "url": "/console/manager/home",
             "activeAppUrl": "/console",
             "hasRole": "ROLE_SUPERUSER"
+          },
+          {
+            "label": "Utilisateurs",
+            "url": "/console/manager/browse/all/users",
+            "activeAppUrl": "/console/manager/browse/all/users",
+            "hasRole": "ROLE_SUPERUSER"
+          },
+          {
+            "label": "Organismes",
+            "url": "/console/manager/orgs/all",
+            "activeAppUrl": "/console/manager/orgs/all",
+            "hasRole": "ROLE_SUPERUSER"
           }

it should be /import/ for datafeeder entry (untested yet as i have to inject geor-header in DF UI)

-        "url": "/datafeeder",
-        "activeAppUrl": "/datafeeder",
+        "url": "/import/",
+        "activeAppUrl": "/import/",

something cleverer should be done for GN-UI, otherwise an user having roles for both will get two hilighted entries. for now trying with "activeAppUrl": "includes:catalog.edit#/board" and "activeAppUrl": "end:admin.console" but i have to unbreak my GN which got updated from 4.2.2 to 4.2.8.

landryb · 2024-12-10T13:31:03Z

when being on mapstore (thus the corresponding entry is highlighted) and clicking on the cadastrapp entry, it correctly loads the cadastre context, but the mapstore item is still hilighted, and the cadastrapp entry isnt. It is only hilighted properly when reloading the complete page. I .. guess that's a side effect of the SPA.

f-necas · 2024-12-10T14:36:57Z

Indeed, the # act as a hash and break the change of tab. maybe I can trigger a recheck after each click

landryb · 2024-12-11T07:56:42Z

im not sure 8a2ef79 is 'the right way' because it's quite glitchy, at least for the mapstore test case:

first it shows very shortly (eg 0.5ms) a MS mapstore error about a failure to load the context
then it proceeds a full reload of the page (including the header), which is .. lengthy. pretty sure that wasnt the case before, eg it very quickly loaded the context without a full reload

from my little understanding of this modern JS thing, the header keeps a 'state' when it stays 'on the same page' (eg in a SPA, be it mapstore or GN admin or GN search results). Can't we use that to remember which entry is highlighted, and if the new-to-be-hilighted-entry is different/changed, remove the highlighting on the previous one/reset all existing highlighting ?

landryb · 2024-12-19T10:50:27Z

still testing this PR, the stylesheet and logoURL entries from the config object in config.json doesnt take precedence/overrides the one set in $DATADIR/default.properties here...

looking at the code, my understanding is that it should, but from testing, if the index.html contains <geor-header id="georchestra-header" config-file="/dist/config.json"> in the webapp, the rendered html has

<geor-header id="georchestra-header" config-file="/dist/config.json" active-app="mapstore" legacy-url="/header/" legacy-header="false" style="height:90px" logo-url="/dist/logo.svg" stylesheet="/dist/stylesheet.css"></geor-header>

where the config keys for logo/stylesheet are the ones coming from default.properties. Or that's multiple mechanisms fighting each other to change the same DOM node ?

landryb · 2024-12-19T12:01:27Z

meh, yet again got bit by having the header configuration in mapstore's localConfig.json .. my understanding of this PR will mean that we'll be able to drop most of the content of the header plugin, even drop it completely ? trying to remove it from localConfig shows only an empty header div but the <geor-header> DOM node is present.

landryb · 2024-12-19T14:30:21Z

another side-effect, we're losing the CatalogIcon UserIcon SomethingIcon web-components that were present in the default admin-menu, how should those get configured ? via font-awesome icons/css ?

f-necas · 2024-12-20T08:13:01Z

Yep I need to edit every georchestra's app to remove attribute and set config file one. The stylesheet and active this still be present as they are used in other apps or in legacy header. https://github.com/georchestra/header/pull/25/files#diff-a0d1c9a9f2eec4ee7507a995e0254b610e5d97262285b35e057980c9ca50a159L17-L23

For the icons, I temporarily decided to remove it, as I didn't find an elegant way to introduce it.
It could be a link added in config file (in order to import it just if necessary)

arnaud-morvan · 2024-12-20T20:42:40Z

src/header.ce.vue

+  state.loaded = true
+}
+
+function matchLinkUrl(link: Link): boolean {


IMHO cannot survive without tests, cannot be understood without tests.
Desserve some comments.

Maybe you could create a pure function (value returned only depends on arguments) easily testable outside component.

f-necas force-pushed the modular-conf branch from 6bd3300 to b98672b Compare October 4, 2024 14:41

f-necas added 4 commits December 6, 2024 16:50

feat: implement first step of modular

e5196b7

feat: first step of condition

4ebe0c0

feat: set menu config file and block account

47aa4a1

feat: add i18n, config file, improve repsonsive

0a49d94

f-necas force-pushed the modular-conf branch from 4ccbd1d to 0a49d94 Compare December 6, 2024 15:51

f-necas marked this pull request as ready for review December 9, 2024 08:12

feat: add active app url

a42e38b

feat: add types and update active app scoring

f1cd390

f-necas added 2 commits December 10, 2024 10:52

feat: remove activeApp

da82e95

feat: format

b27d85d

feat: this should work

8a2ef79

feat: remove getActiveTab

0445430

feat: better naming

ef47dd2

arnaud-morvan reviewed Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement first step of modular #25

feat: implement first step of modular #25

f-necas commented Aug 2, 2024 •

edited

Loading

landryb commented Dec 2, 2024

landryb commented Dec 9, 2024

f-necas commented Dec 9, 2024

landryb commented Dec 9, 2024 •

edited

Loading

f-necas commented Dec 9, 2024

landryb commented Dec 9, 2024 •

edited

Loading

landryb commented Dec 9, 2024

landryb commented Dec 9, 2024

landryb commented Dec 10, 2024

landryb commented Dec 10, 2024 •

edited

Loading

landryb commented Dec 10, 2024

landryb commented Dec 10, 2024 •

edited

Loading

f-necas commented Dec 10, 2024

landryb commented Dec 11, 2024

landryb commented Dec 19, 2024 •

edited

Loading

landryb commented Dec 19, 2024

landryb commented Dec 19, 2024

f-necas commented Dec 20, 2024

arnaud-morvan Dec 20, 2024 •

edited

Loading

arnaud-morvan Dec 20, 2024 •

edited

Loading

feat: implement first step of modular #25

Are you sure you want to change the base?

feat: implement first step of modular #25

Conversation

f-necas commented Aug 2, 2024 • edited Loading

Custom config

Config

Menu

i18n

landryb commented Dec 2, 2024

landryb commented Dec 9, 2024

f-necas commented Dec 9, 2024

landryb commented Dec 9, 2024 • edited Loading

f-necas commented Dec 9, 2024

landryb commented Dec 9, 2024 • edited Loading

landryb commented Dec 9, 2024

landryb commented Dec 9, 2024

landryb commented Dec 10, 2024

landryb commented Dec 10, 2024 • edited Loading

landryb commented Dec 10, 2024

landryb commented Dec 10, 2024 • edited Loading

f-necas commented Dec 10, 2024

landryb commented Dec 11, 2024

landryb commented Dec 19, 2024 • edited Loading

landryb commented Dec 19, 2024

landryb commented Dec 19, 2024

f-necas commented Dec 20, 2024

arnaud-morvan Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

arnaud-morvan Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

f-necas commented Aug 2, 2024 •

edited

Loading

landryb commented Dec 9, 2024 •

edited

Loading

landryb commented Dec 9, 2024 •

edited

Loading

landryb commented Dec 10, 2024 •

edited

Loading

landryb commented Dec 10, 2024 •

edited

Loading

landryb commented Dec 19, 2024 •

edited

Loading

arnaud-morvan Dec 20, 2024 •

edited

Loading

arnaud-morvan Dec 20, 2024 •

edited

Loading