-
Notifications
You must be signed in to change notification settings - Fork 6
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
Adjust web side for new tooling changes (Key -> Keys and OrderOnPage) #674
base: main
Are you sure you want to change the base?
Conversation
7b2c3ec
to
eefdf2d
Compare
70452a1
to
23ff1d6
Compare
This reverts commit 710418c.
eefdf2d
to
3a9a1ca
Compare
v-on:toggle="!$event.target.open ? expandedTocs.delete(item.key) : expandedTocs.add(item.key)"> | ||
<summary :class="{ | ||
'toc-content' : item.level==0, | ||
'nested' : item.level> | ||
0, | ||
'current-section': currentPage.some(p => p.key == item.key), | ||
'current-section': currentPage.some(p => JSON.stringify(p.keys) == JSON.stringify(item.keys)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that in other places we're using the first key of a page as something of a canonical key for a page. Can we not just use the first key here as well rather than repeatedly stringifying arrays? (and likewise for other occurrences below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That definitely makes sense. My reasoning behind trying to find another implementation is I didn't want to assume Keys was non-empty. My only other idea is to add a helper function to check the arrays length then return first element otherwise null, but I wasn't sure if that was the best implementation. Is that what you're thinking for a better alternative?
Also, it's super cool to get feedback from the main maintainer of Coalesce. It's one of my favorite technologies/ideas, maybe aside from LINQ. So yeah, it's cool to virtually meet you :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be invalid for a page to not have any keys? Should add a check for that when building up the TOC data and throw if there is such a page. Then we know its always safe to grab the first key.
Description
Adding support for new Tooling side update: adjusting from
SiteMapping.Key
toKeys
as well as sorting byOrderOnPage
since the new Tooling update changes how sitemap.json is ordered.Ensure that your pull request has followed all the steps below: