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

convert documentation #1576

Merged
merged 23 commits into from
Aug 14, 2024
Merged

convert documentation #1576

merged 23 commits into from
Aug 14, 2024

Conversation

CobusT
Copy link
Contributor

@CobusT CobusT commented Aug 12, 2024

Resolves #930
Resolves https://github.com/observablehq/observablehq/issues/17499

I picked the Markdown page as the most appropriate, but open to other suggestions. Also, I wanted to explain some of the real obvious problems people will run into, so it is a bit verbose... open to feedback.

@CobusT CobusT requested a review from Fil August 12, 2024 20:33
@mbostock
Copy link
Member

mbostock commented Aug 12, 2024

Should this live in the Observable documentation rather than here (since it’s specific to notebooks)?

@CobusT
Copy link
Contributor Author

CobusT commented Aug 12, 2024

Should this live in the Observable documentation rather than here (since it’s specific to notebooks)?

Perhaps... it is a cross-over topic. I thought of adding it here since it is a CLI command that gets installed as part of Framework.

@CobusT
Copy link
Contributor Author

CobusT commented Aug 12, 2024

Thanks for the edits, @Fil

@Fil
Copy link
Contributor

Fil commented Aug 12, 2024

I've made a few corrections (importantly: you run this command wherever you want, but it will save the files at the root of the project), as well as stylistic edits.

This feels a bit too detailed for this documentation, and also not detailed enough for a blog post (there are many more things to cover, like async generators, dark mode, etc).

I would have aimed for a smaller format, which could read as:

## Converting notebooks

To help you convert a public Observable notebook to Observable Markdown, Framework includes the `observable convert` utility. See [………] for details and guidance / tips.

docs/markdown.md Outdated Show resolved Hide resolved
@Fil
Copy link
Contributor

Fil commented Aug 12, 2024

Note: I like the idea of giving concrete examples and showing some suggestions to fix the issues that come up.

We can discuss if converting chart to an arrow function then using display(chart()) in a different code block is the best approach, or if we should (for example) recommend replacing return svg.node(); by display(svg.node());. When porting pangea I started with the first approach (which can be said to be more generic), but for more recent pages I kind of moved to the second approach (removing complexity and indirections).

@CobusT
Copy link
Contributor Author

CobusT commented Aug 12, 2024

Perhaps the solution is to go with your very short answer and then point them to a more detailed page/section in the platform docs with the example? I don't think the example needs to be exhaustive, but I did want to show some of the things that would need to be done.

@mbostock
Copy link
Member

It should be its own page, at least. I don’t think it’s appropriate to drop it in the Markdown documentation where it will be a distraction for most readers.

@CobusT
Copy link
Contributor Author

CobusT commented Aug 13, 2024

Options I see:

  1. Keep a short one-sentence mention of this on the Framework Markdown page and create a page with the example in the Platform docs under the Projects section (soon to be Data Apps) with the example.
  2. Don't mention it in Framework docs and create a new page on the Platform docs (under Projects/Data apps)
  3. Something else...

@mbostock
Copy link
Member

As I said, I don’t think it’s appropriate to talk about the convert command on Framework’s Markdown documentation. The convert command has very little to do with Framework’s Markdown syntax and features and isn’t relevant to people who are authoring Framework pages. It’s only relevant to people that are converting from notebooks, so it should be considered a separate topic/task.

Therefore I think it either lives as a standalone page in the Framework docs (as deploying does), or a standalone page in the Observable docs. I guess it should be a standalone page in the Framework docs because then we can update the docs if we change the behavior of the command.

I’ll also note that we could implement better observable help convert (and other commands), too.

@CobusT
Copy link
Contributor Author

CobusT commented Aug 13, 2024

OK, thanks. I will move this to a standalone page in the Framework docs.

@CobusT
Copy link
Contributor Author

CobusT commented Aug 13, 2024

Added a new page 'Converting notebooks' after the Deploying page in the left nav, and made some small edits.

@CobusT CobusT requested a review from Fil August 13, 2024 05:06
@CobusT
Copy link
Contributor Author

CobusT commented Aug 13, 2024

@Fil @mbostock hopefully this is good to go...

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

This is a fine start. I’d like to expand this a bit to cover the limitations (syntax, imports, private notebooks, standard library differences, etc.). I’ll push some commits when I get a chance.

@mbostock
Copy link
Member

I’ll try to do it tonight after dinner I guess. I think it needs more work to cover the topics I mentioned. We could always delay the blog post if linking to the documentation is a requirement, but I’d rather get this into a better state before advertising it.

@CobusT
Copy link
Contributor Author

CobusT commented Aug 13, 2024

@mbostock I added a section at the bottom ('Things to note' - didn't want to call them limitations...), probably more a placeholder than anything else, since I am not sure how much you want to expand on these. They are probably also not exactly right, sorry. Just wanted to get it closer to what you envisioned. Hopefully that helped.

Thanks for looking at this.

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

I’m going to sleep now, but this needs a few hours more work. I’ve drafted an outline of the sections I want to cover, but there is a lot of detail to fill in. I’ll try to do more work tomorrow but I can’t guarantee it will be done.

Also, an important note on tone: it’s critical that we emphasize that Framework uses vanilla JavaScript and CommonMark syntax, unlike notebooks which use (nonstandard) Observable JavaScript syntax. Terminology such as “Framework’s JavaScript syntax” suggests that Framework is nonstandard, and that the differences are arbitrary; it’s a big deal that Framework now uses standard syntax for both JavaScript and Markdown and we should celebrate it!

@mbostock mbostock self-assigned this Aug 14, 2024
@mbostock
Copy link
Member

To-do: also describe limitations around cell modes (and in particular data table cell and chart cell), database connectors, secrets…

docs/convert.md Outdated Show resolved Hide resolved
@Fil
Copy link
Contributor

Fil commented Aug 14, 2024

I've added a few elements of information; it's still missing details but maybe it's good enough to start.

@mbostock
Copy link
Member

Appreciate the help @Fil but please hold off on further contributions as I’m still rewriting everything.

@mbostock
Copy link
Member

mbostock commented Aug 14, 2024

We can discuss if converting chart to an arrow function then using display(chart()) in a different code block is the best approach, or if we should (for example) recommend replacing return svg.node(); by display(svg.node());.

I’m strongly in favor of the latter approach and that’s what I’m now recommending in this doc.

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

Would appreciate another pair of eyes on all those edits but I think this is now good from my end. 👍 Thanks for your patience and for everyone’s contributions!

@CobusT
Copy link
Contributor Author

CobusT commented Aug 14, 2024

It looks good to me, too. Thanks for writing all of this!

@CobusT CobusT merged commit 52293fb into main Aug 14, 2024
4 checks passed
@CobusT CobusT deleted the cobus/convert-documentation branch August 14, 2024 20:08
@Fil Fil mentioned this pull request Aug 15, 2024
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.

document observable convert
3 participants