Fix node-processing bug from dropdown temporary placeholder #1467
Conversation
packages/core/src/plugins/algolia.js
Outdated
| 'question div[slot=hint]', | ||
| 'question div[slot=answer]', | ||
| 'tab:not(:first-child)', | ||
| 'tab:nth-of-type(n+2)', |
There was a problem hiding this comment.
I'm assuming that we want to target all the instances of tab except for the first instance. As slot processing will introduce a new template node (which will become the first child instead of tab), this selector will not be correct.
I think what we should be selecting is the second instance of tab onwards, and this selector lets us achieve that.
There was a problem hiding this comment.
let's move this (and above) to the other pr if necessary since we're just fixing the temp fouc styles here,
but both seem like separate pre-existing issues to me
For the second, it may be intended behaviour (e.g. in here https://markbind-master.netlify.app/userguide/usingcomponents#tabs - only the first (visible) one under <tabs> should be indexed)
Could look up the algolia pr, or git annotate this change to double check
There was a problem hiding this comment.
Alright! I've made the changes :-)
packages/core/src/plugins/algolia.js
Outdated
| 'question div[slot=hint]', | ||
| 'question div[slot=answer]', | ||
| 'tab:not(:first-child)', | ||
| 'tab:nth-of-type(n+2)', | ||
| 'tab-group:not(:first-child)', |
There was a problem hiding this comment.
Should we use nth-of-type(n+2) here as well? I don't see problems here currently as we are not introducing new nodes (e.g. template) to become the first child, but I'm thinking nth-of-type(n+2) may be a more future-proof way to select the node.
There was a problem hiding this comment.
nice catch here, seems like several of the selectors could use some refining overall
Let's move that to a different PR if you're up for it 🙂
There was a problem hiding this comment.
Sure! will make a PR for that :-)
wxwxwxwx9
changed the title
|
Hi @ang-zeyu, can I get ur help to review this PR? :-) |
This reverts commit 09bf5ac.
What is the purpose of this pull request?
Overview of changes:
This issue was discovered in #1464. And this fix is needed for #1464 to continue.
Moved the dropdown temporary placeholder after the
<dropdown>node itself. This is because having it before (as it is currently) would interfere with the processing of the nodes and it can be seen from the expected html file fortest_site_algolia_expectedthat the nodes after dropdown are not processed properly. There are also a lot of duplicated<div class="temp-dropdown-placeholder"></div>.Moving the placeholder after would allow the nodes after
<dropdown>to be processed properly and this also means that we will need to update the algolia selectors (which will be done in a separate PR).Anything you'd like to highlight / discuss:
If the method of resolution is appropriate.
Testing instructions:
npm run testProposed commit message: (wrap lines at 72 characters)
Fix node-processing bug from dropdown temporary placeholder.
Inserting the dropdown temporary placeholder before the dropdown itself
introduces some node processing bug, where nodes (e.g. panel) are not
processed properly after dropdown. Furthermore, it generates a lot of
duplicated dropdown temporary placeholder. This issue can be seen from
the test in
test_site_algolia_expected.Let's insert the dropdown temporary placeholder after the dropdown node
to circumvent this problem. Having it after the dropdown node will ensure
that the node processing is executed in a normal / fixed processing order.
Checklist: ☑️