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

Theory-specific help #269

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Theory-specific help #269

wants to merge 1 commit into from

Conversation

hamidahoderinwale
Copy link
Collaborator

@hamidahoderinwale hamidahoderinwale commented Nov 23, 2024

Theory-specific help

  • Help button directs to main help page when a theory hasn't been selected
  • Otherwise it directs to a theory-specific help page

(PR is incomplete, routing isn't working)

@epatters epatters changed the title updated commit, remove files Theory-specific help Nov 23, 2024
@epatters epatters added enhancement New feature or request frontend TypeScript frontend and Rust-wasm integrations ui/ux User interface and user experience labels Nov 23, 2024
@epatters epatters marked this pull request as ready for review November 26, 2024 01:16
@epatters epatters marked this pull request as draft November 26, 2024 01:16
Copy link
Member

@epatters epatters left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started, Hamidah! I've provided some initial feedback below. Let's discuss more in person.

@@ -45,6 +45,8 @@ export function TheorySelectorDialog(
);
}

export const [selectedTheory, setSelectedTheory] = createSignal<string | undefined>(undefined);
Copy link
Member

@epatters epatters Nov 26, 2024

Choose a reason for hiding this comment

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

I don't know if creating a signal outside of a component, for example at the top level like here, will work. More importantly, we shouldn't need to create a new signal at all. The selected theory is already available from the LiveModelDocument. See below.

<CircleHelp />
</IconButton>
);
if (selectedTheory() == null) {
Copy link
Member

@epatters epatters Nov 26, 2024

Choose a reason for hiding this comment

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

Instead of using selectedTheory(), create a TheoryHelpButton that takes the theory ID (or undefined) as a prop. Then pass the theory as props.liveModel.theory() from inside the ModelDocumentEditor.

@@ -0,0 +1,46 @@
import { readdirSync, statSync, writeFileSync } from "node:fs";
import { join, resolve } from "node:path";
Copy link
Member

Choose a reason for hiding this comment

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

This will not work because you're using Node-specific APIs, which are not available in the browser. That's why the CI is failing. We can discuss alternative approaches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request frontend TypeScript frontend and Rust-wasm integrations ui/ux User interface and user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants