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

Creating an IndexStore for Quick Navigation #889

Merged
merged 27 commits into from
Sep 16, 2024

Conversation

hqhhuang
Copy link
Contributor

Bug/issue #, if applicable: rdar://134272215

Summary

  1. This PR creates a new IndexStore. The eventual goal is to share this store between the navigator and Quick Navigation.
    But for this PR, only Quick Navigation will be getting data from the new store.

  2. Additionally, the index data is fetched from the page view instead.

  3. Moves IncludedArchiveIdentifiers data from AppStore to the new IndexStore.

Testing

Manual testing, verify that Quick Nav still behaves as expected.
I've added some tests. I'm working on adding more.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

  • Added tests
  • Ran npm test, and it succeeded
  • Updated documentation if necessary

Create `IndexStore`
fetch index data in `view/DocumentationTopic`
Get Quick Nav data from `IndexStore`
update items in `indexStore`
add `technologyUrl` prop
fix issues with accessing null items
move data fetching logic to `indexProvider.js`
hqhhuang and others added 5 commits August 19, 2024 11:38
add `errorFetching` to store
simplify index data path computation logic
add technology prop to store
add tests for `indexProvider`
@hqhhuang
Copy link
Contributor Author

@swift-ci test

@hqhhuang hqhhuang marked this pull request as ready for review August 23, 2024 21:51
@marinaaisa
Copy link
Member

marinaaisa commented Aug 26, 2024

I've been checking the code and testing Quick Navigation and it's looking good to me!

The only thing that I don't like in this current state is that we are duplicating the NavigatorDataProvider computed properties into the indexProvider but I don't see any other way of not doing it until we get rid of the NavigatorDataProvider in the next step.

resets store before fetching new indexData
@hqhhuang hqhhuang changed the title [WIP] Creating an IndexStore for Quick Navigation Creating an IndexStore for Quick Navigation Aug 29, 2024
src/components/DocumentationLayout.vue Outdated Show resolved Hide resolved
src/components/DocumentationTopic.vue Outdated Show resolved Hide resolved
src/components/DocumentationTopic.vue Outdated Show resolved Hide resolved
src/components/Navigator/NavigatorDataProvider.vue Outdated Show resolved Hide resolved
src/mixins/indexProvider.js Show resolved Hide resolved
src/views/DocumentationTopic.vue Outdated Show resolved Hide resolved
@hqhhuang hqhhuang changed the base branch from main to navigator-index-store September 3, 2024 23:00
do not set `includedArchiveIdentifiers` in Navigator Provider
rename state to `indexState`
simplify logic to find technology index data
remove url related computed properties
move fetching logic to `view/DocumentationTopic`
display index data for correct language variant
save flattened data for all language variants to store
…render into index-store"

This reverts commit 3dfcb67, reversing
changes made to cbb7f50.
update flatChildren default format
make sure data defaults to swift variant & add tests
Copy link
Contributor

@mportiz08 mportiz08 left a comment

Choose a reason for hiding this comment

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

This seems cleaner to me overall as a first step of moving this data to a shareable store. 👍

Thanks for working through all these complex layers!

src/components/DocumentationLayout.vue Outdated Show resolved Hide resolved
src/components/DocumentationLayout.vue Outdated Show resolved Hide resolved
rename and clean up node logic
tests/unit/mixins/__snapshots__/indexProvider.spec.js.snap Outdated Show resolved Hide resolved
tests/unit/mixins/indexProvider.spec.js Show resolved Hide resolved
tests/unit/mixins/indexProvider.spec.js Outdated Show resolved Hide resolved
tests/unit/stores/IndexStore.spec.js Outdated Show resolved Hide resolved
@hqhhuang hqhhuang changed the base branch from navigator-index-store to main September 11, 2024 22:48
Improve tests and test description
@hqhhuang hqhhuang changed the base branch from main to navigator-index-store September 11, 2024 23:00
expect(wrapper.find(QuickNavigationModal).props('children')).toEqual(swiftChildren);
});

it('QuickNavigation falls back to swift items, if no objc items', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we fallback to swift items? just curious about this

Copy link
Contributor Author

@hqhhuang hqhhuang Sep 16, 2024

Choose a reason for hiding this comment

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

See this issue, DocC defaults to swift when the archive is article only. If this issue is addressed, we can make the update here as well.

Copy link
Member

@marinaaisa marinaaisa left a comment

Choose a reason for hiding this comment

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

Reviewed and tested. LGTM! thanks for working on this!

@hqhhuang hqhhuang merged commit e787f8b into swiftlang:navigator-index-store Sep 16, 2024
@hqhhuang hqhhuang deleted the index-store branch September 16, 2024 16:59
This pull request was closed.
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.

3 participants