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

infoj_order mode #1252

Merged
merged 22 commits into from
May 13, 2024
Merged

infoj_order mode #1252

merged 22 commits into from
May 13, 2024

Conversation

dbauszus-glx
Copy link
Member

@dbauszus-glx dbauszus-glx commented May 10, 2024

The infoj method has a second infoj_order argument. This argument can be an array of keys and entry objects.

layer.infoj entries will be filtered in the order of keys. It is checked whether the key matches the entry's key, field, query, or group key value.

The key key value is specifically used as an identifier for entries and serves no other purpose.

The infoj_order argument can be a mix of entry objects and keys. Entry object are directly spliced into the infoj array and don't require to be in the layer.infoj array.

The infoj_order can be defined as an array in the layer json. The infoj_order plugin will be retired in favour of this.


simon-leech and others added 13 commits May 10, 2024 15:52
1. Delete entry.run once running to prevent multiple runs
2. Add entry.hasRan = true to ensure warning if not fired incorrectly
3. Fix issue where a response with no value to set could not be skipNullValue : true
4. Only appendChild (replaceChildren was removing the title when using nullValue = 'none' for example)
@cityremade
Copy link
Member

when including a dataview object in the order I see the following error:
infoj.mjs:130 Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'layer')
at line 130 in infoj.mjs: entry.location.layer.queryparams || {}
Entry in the order array doesn't inherit properties when they are referenced.

@@ -114,7 +114,7 @@ function entryDefault(entry) {

function entryTitle(entry) {

if (entry.title) return;
if (!entry.title) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Noticed an issue here where if the entry had a title it would return.

Was just missing the operator.

Copy link
Contributor

@KieronFerrey KieronFerrey left a comment

Choose a reason for hiding this comment

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

Tested with @simon-leech - so throwing feedback here.

  1. Can we add entry.type to the options - as this enables plugins that add elements to the infoj to be ordered.
  2. entry.group is not working correctly - this is possibly due to the fact that we aren't looping over the infoj and looking for every option that has that group - so it only returns one entry in each group.
  3. dataviews in the infoj don't seem to be ordering

@dbauszus-glx
Copy link
Member Author

@KieronFerrey group won't work since the values are not unique. tyoe won't work neither for the same reason. we need to remove the group key since sorting requires a unique identifier.

@simon-leech
Copy link
Contributor

I just pushed a commit to this PR after chatting to @RobAndrewHurst.
This PR simply console warns the developer if a value provided in infoj_order array does not exist in the location.infoj.
This is required to ensure that the developer added a unique identifier to plugin entries or other entries that may not be unique.

Copy link
Contributor

@AlexanderGeere AlexanderGeere left a comment

Choose a reason for hiding this comment

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

Looks good to me, Can order dataviews as well!

@dbauszus-glx
Copy link
Member Author

I have removed group from the infoj_order lookup since this is by definition not unique.

@simon-leech
Copy link
Contributor

I just pushed a check on the mapview and locale - as is custom views called entries may not have a locale

// Assign queryparams from layer, and locale.
  entry.queryparams = Object.assign(
    entry.queryparams || {},
    entry.location.layer.queryparams || {},
    entry.location.layer?.mapview?.locale?.queryparams || {})

Copy link

sonarcloud bot commented May 13, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@dbauszus-glx
Copy link
Member Author

@simon-leech Can you test whether this works using the spread operator?

image

Aren't the locale queryparams already assigned to the layer somewhere else?

@simon-leech
Copy link
Contributor

@dbauszus-glx Yeah all seems to be working for me :)

Copy link
Contributor

@KieronFerrey KieronFerrey left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Contributor

@RobAndrewHurst RobAndrewHurst left a comment

Choose a reason for hiding this comment

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

Lets go!

@RobAndrewHurst RobAndrewHurst merged commit 1de14d9 into GEOLYTIX:main May 13, 2024
5 checks passed
@dbauszus-glx
Copy link
Member Author

@dbauszus-glx dbauszus-glx deleted the infoj_order branch September 9, 2024 10:41
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.

6 participants