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

Cosmetic fixes to get-started #102

Merged
merged 3 commits into from
Nov 22, 2024
Merged

Cosmetic fixes to get-started #102

merged 3 commits into from
Nov 22, 2024

Conversation

falexwolf
Copy link
Member

@falexwolf falexwolf commented Nov 22, 2024

Cosmetic fixes. See discussion points in the PR below:

@falexwolf falexwolf changed the title Cosmetic fixes Cosmetic fixes to get-started Nov 22, 2024
@@ -55,7 +55,6 @@ Create your default database `db` object for this R session:

```{r connect-default}
db <- connect()
db
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed it because it confusingly displays cellxgene -- see discussion in other PR.

@falexwolf
Copy link
Member Author

One more thing needs to be fixed:

image


# Connect to the default instance
# Start an R session
Copy link
Collaborator

Choose a reason for hiding this comment

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

I changed this before as I find "Start an R session" confusing as users already need to have an R session to run the commands

Copy link
Member Author

Choose a reason for hiding this comment

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

But then we need another heading. We can call it "Start an analysis session in R".

The commands above are all on the terminal except the install.packages line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or "Start your analysis"

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll make this fix.

@@ -25,7 +25,7 @@ This vignette introduces the basic **{laminr}** workflow.
Install **{laminr}** from CRAN:

```r
install.packages("laminr")
install.packages("laminr", dependencies = TRUE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I usually wouldn't suggest doing this here but it's fine for now

Copy link
Member Author

Choose a reason for hiding this comment

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

We have it in the quickstart and in the demo instruction.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we don't have it here, it'd be inconsistent with the other training material.

The goal is to have a one line installation command and not talk through different ways of installing the package.

@falexwolf
Copy link
Member Author

Demo starts in 2 minutes, can you please merge this @lazappi ?

@lazappi lazappi merged commit 969b610 into main Nov 22, 2024
7 checks passed
@lazappi lazappi deleted the fixes branch November 22, 2024 14:00
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