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

fix navigation issues with spec #2311

Merged
merged 1 commit into from
Mar 20, 2025
Merged

Conversation

tintinthong
Copy link
Contributor

@tintinthong tintinthong commented Mar 20, 2025

This PR fixes the issue that if you change a selection on LHS, it doesn't remain stale (particularly after you have chosen something in the dropdown) See this test

Other changes

  • moved out of pre-rendered card search bcos shudn't rely on pre-rendered card to return meta data (so I used getSearch)
  • selected spec is persisted
  • any writes to playground service is done thru a modifier since we shudn't rely on the readiness of the search resource before commiting the persisted playground state
  • once you create a spec, it will open the accordion if its not already opened

As part of this PR there is a digression in auto-save. I do not have a good solution for that and have skipped the test. But, the structure that we want is closer to this. PLS REVIEW THIS INDEPENDENTLY OF THE AUTO_SAVE ISSUE

@tintinthong tintinthong force-pushed the fix-first-instance-change-file-2 branch from 5b9a88b to c2986af Compare March 20, 2025 01:49
@tintinthong tintinthong changed the base branch from cs-8185-refactor-getcard-usage to main March 20, 2025 01:49
Copy link

github-actions bot commented Mar 20, 2025

Host Test Results

  1 files  ±0    1 suites  ±0   27m 45s ⏱️ -17s
840 tests +1  835 ✅ ±0  5 💤 +1  0 ❌ ±0 
845 runs  +1  840 ✅ ±0  5 💤 +1  0 ❌ ±0 

Results for commit 3129465. ± Comparison against base commit 8e1fed4.

This pull request removes 1 and adds 2 tests. Note that renamed tests count towards both.
Chrome 134.0 ‑ Acceptance | Spec preview: spec auto saved
Chrome 134.0 ‑ Acceptance | Spec preview: spec auto saved (with stability)
Chrome 134.0 ‑ Acceptance | Spec preview: spec preview updates when changing between different declarations inside inspector

♻️ This comment has been updated with latest results.

@tintinthong tintinthong requested review from richardhjtan, lucaslyl and a team March 20, 2025 08:07
@onClick={{fn
this.selectAccordionItem
'schema-editor'
false
Copy link
Contributor

Choose a reason for hiding this comment

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

@tintinthong is this boolean parameter needed?
@action private selectAccordionItem(item: SelectedAccordionItem) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

this.selectAccordionItem
'playground'
}}
@isLoadingNewModule={{this.moduleContentsResource.isLoadingNewModule}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This might not be needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do want to keep this so I can show a loading state while changing module and return a default title

accordion fix

fix lint

wrong argument

fix test

fix test

follow up fixes

fix lint

fix lint

fix lint

fix lint
@tintinthong tintinthong force-pushed the fix-first-instance-change-file-2 branch from fd953b9 to 3129465 Compare March 20, 2025 11:13
@tintinthong tintinthong requested a review from a team March 20, 2025 12:19
Copy link
Contributor

@burieberry burieberry 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

@tintinthong tintinthong merged commit 7ced2c8 into main Mar 20, 2025
48 checks passed
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