Skip to content

Set loading state when switching between modes#2795

Merged
koesie10 merged 2 commits intomainfrom
koesie10/switch-mode-loading
Sep 8, 2023
Merged

Set loading state when switching between modes#2795
koesie10 merged 2 commits intomainfrom
koesie10/switch-mode-loading

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Sep 8, 2023

This sets the loading state when switching between application and framework mode. It does this by setting the methods to an empty array, which will show the loading state (methods.length === 0).

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@koesie10 koesie10 requested a review from a team as a code owner September 8, 2023 11:41
t: "switchMode",
mode: newMode,
});
setMethods([]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In practice this works just fine.

The only thing I'm wondering is if it's ok that the methods in ModelEditor and in ModelEditorView are getting temporarily out of sync. Another option could be that when handling the switchMode message, we set ModelEditorView.methods = [] and emit a setMethods message. Ultimately it doesn't matter much because we then call loadExternalApiUsages which updates the methods.

In this case I think this is ok because the methods get recomputed immediately. But it is something to be aware of generally that we need to keep this state in sync.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point, I think setting the methods on the ModelEditorView is better, so I'll update the PR to do that instead.

@koesie10 koesie10 merged commit b141291 into main Sep 8, 2023
@koesie10 koesie10 deleted the koesie10/switch-mode-loading branch September 8, 2023 14:54
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.

2 participants