Bugfix/navbar entries order #3334
Conversation
Hello jbwatenbergscality,My role is to assist you with the merge of this Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list:
|
JBWatenbergScality
force-pushed
the
bugfix/navbar-entries-order
branch
from
7087508 to
f2abf2e
Compare
JBWatenbergScality
force-pushed
the
bugfix/navbar-entries-order
branch
from
f2abf2e to
146536d
Compare
| throw new Error( | ||
| `[navbar][config] Invalid path specified in "options.${section}": "${path}" ` + | ||
| '(keys must be defined as fully qualified URLs, ' + | ||
| 'such as "{protocol}://{host}{path}?{queryParams}")', | ||
| ); |
There was a problem hiding this comment.
Now realizing this try/catch is likely too wide for the error message it raises... Let's tackle this as debt.
| expect(entries).toStrictEqual(['First', 'Second', 'Third']) | ||
| }); | ||
|
|
||
| it('should enqueue unordered entries', async () => { |
There was a problem hiding this comment.
How can this work with:
entries.sort(([_, entryA], [__, entryB]) => (entryA.order - entryB.order));??? 🤔 Javascript is really a mistery to me...
There was a problem hiding this comment.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/entries
The order of the array returned by Object.entries() does not depend on how an object is defined. If there is a need for certain ordering, then the array should be sorted first, like Object.entries(obj).sort((a, b) => b[0].localeCompare(a[0]));.
There was a problem hiding this comment.
Oh, that was not my question actually @ChengYanJin (but it confirms that we should really not rely on default ordering).
My point was that we are sorting entries based on entryA.order - entryB.order... But we are able to handle entries for which entryC.order === undefined, and somehow they end up at the end (which this test is verifying). This part is really mystical to me.
There was a problem hiding this comment.
OK, tested it a bit (see https://jsfiddle.net/Loxm9ck4/2/), it's actually broken, the sort logic just doesn't move elements when faced with such a case (the comparison function returns NaN). So I think your test is somewhat broken @JBWatenbergScality 😕
There was a problem hiding this comment.
(Typically something that would have been caught immediately with typescript)
|
/approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue None. Goodbye jbwatenbergscality. |
Component:
UI
Context:
We want to provide a way for users to define a specific order of appearance for the navbar entries.
Summary:
Adding an optional
orderproperty to define the order of entries.Acceptance criteria:
Unit tests