Skip to content

Components #522

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

Merged
merged 39 commits into from
Jan 18, 2024
Merged

Components #522

merged 39 commits into from
Jan 18, 2024

Conversation

allisonhorst
Copy link
Contributor

@allisonhorst allisonhorst commented Jan 12, 2024

Will eventually include docs for:

Also TODO:

  • Move static files (e.g. horse.jpg) to docs/data
  • Ensure cut components are removed (pages and sidebar items, e.g. annotation, bignumber)

Planning doc

Allison Horst and others added 14 commits January 8, 2024 18:06
* update button.md
* update markdown text section
* first draft inputs overview

* updates nav with javascript/inputs

* Apply suggestions from Fil review

Co-authored-by: Philippe Rivière <[email protected]>

* updates to use olympians, uses lowercase and 'inputs' throughout

* case fixes and minor update

Co-authored-by: Philippe Rivière <[email protected]>

---------

Co-authored-by: Allison Horst <[email protected]>
Co-authored-by: Philippe Rivière <[email protected]>
@allisonhorst allisonhorst requested a review from Fil January 12, 2024 16:23
@allisonhorst allisonhorst marked this pull request as draft January 12, 2024 16:24
@Fil
Copy link
Contributor

Fil commented Jan 12, 2024

  • remove the athletes.csv and penguins.csv files
  • lowercase the Inputs
  • the File input is now fully working
  • "check if pointing to d3-group notebook is best" => no, prefer https://d3js.org/d3-array/group

Copy link
Member

@mbostock mbostock Jan 15, 2024

Choose a reason for hiding this comment

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

This should start with a generic description of what “inputs” are within the concept of the Observable CLI before introducing “Observable Inputs” as a specific solution. It should mention the concepts (and link to) the concepts of reactive top-level variables, generators, and the display function, and then first introduce the view function as how inputs are declared. We should also mention that Generators.input is used under the hood, and link to that too (see Observable Generators).

Only after these concepts are introduced should be introduce Observable Inputs as a library (an implementation) of commonly-needed input components, each of which is a function that returns an HTML element that emits input events (for compatibility with view and Generators.input).

So, yes, the first code snippet of actual usage should be view + Inputs.whatever, so that people see what the recommended usage pattern is. But the text needs to explain what’s happening so that people have a conceptual understanding of how it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updates here: #545 (merged into Components branch, top-level Inputs page)

Copy link
Member

Choose a reason for hiding this comment

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

This is already available in docs/lib and we shouldn’t duplicate it. We could move (static) data files to docs/data, but that’ll mean a few more relative paths in the examples. Maybe that’s fine… and it would be consistent with what we now recommend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could move (static) data files to docs/data, but that’ll mean a few more relative paths in the examples. Maybe that’s fine… and it would be consistent with what we now recommend.

I think we should do this.

Copy link
Member

Choose a reason for hiding this comment

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

To align with Plot’s mental model, perhaps better to combine this and the hexbin example into a bin.md that demonstrates group, bin, and hexbin transforms?

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 like this.

I am stuck on naming. The existing sidebar items and page titles are general chart types (e.g. "Line", "Area" for nav, "Line charts", "Area charts" for page title).

Here, that could be something like:

  • Sidebar: "Aggregation", "Summary"
  • Page title: "Summary charts", "Aggregated data", "Aggregated charts", "Charts with aggregated/binned/grouped data" (?)

Recommendations for naming this section (in the sidebar, and page title) are appreciated.

Copy link
Contributor

Choose a reason for hiding this comment

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

my suggestion: "Grouping data"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add "Grouping data" page with histogram, hexbin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in #560

Allison Horst and others added 6 commits January 15, 2024 11:51
* update inputs-overview intro text

* links

* remove h2 for intro

* adds Inputs general intro, update view documentation in Display page

* Update view summary in javascript/display

Co-authored-by: Philippe Rivière <[email protected]>

* Apply suggestions from code review

Co-authored-by: Philippe Rivière <[email protected]>

---------

Co-authored-by: Allison Horst <[email protected]>
Co-authored-by: Philippe Rivière <[email protected]>
Allison Horst and others added 2 commits January 17, 2024 17:19
Adds `card` class 

---------

Co-authored-by: Allison Horst <[email protected]>
Co-authored-by: Robert Harris <[email protected]>
Co-authored-by: Cindy <[email protected]>
Co-authored-by: Philippe Rivière <[email protected]>
@@ -15,10 +16,58 @@ export default {
{name: "Imports", path: "/javascript/imports"},
{name: "Files", path: "/javascript/files"},
{name: "Promises", path: "/javascript/promises"},
{name: "Inputs", path: "/javascript/inputs"},
Copy link
Member

Choose a reason for hiding this comment

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

Let’s move this to after Display. (I don’t want to interrupt Promises and Generators, which are closely related in terms of implicit resolution of values across code blocks, and Inputs are closely related to Display.)

allisonhorst and others added 8 commits January 18, 2024 09:12
Adds brief overview of components (layout, charts, inputs)

---------

Co-authored-by: Allison Horst <[email protected]>
Co-authored-by: Philippe Rivière <[email protected]>
* add Grouping data section with hexbin, histogram

* adds Grouping data page to sidebar in Charts

* minor fixes and update hexbin chart to use dot mark

* Apply suggestions from code review

Co-authored-by: Philippe Rivière <[email protected]>

---------

Co-authored-by: Allison Horst <[email protected]>
Co-authored-by: Philippe Rivière <[email protected]>
* add grid doc

* change how example code is rendered

* remove word salid

* improve advice

* fix capitolization

* Update docs/layout/grid.md

Co-authored-by: Philippe Rivière <[email protected]>

* Update docs/layout/grid.md

Co-authored-by: Philippe Rivière <[email protected]>

* Update docs/layout/grid.md

Co-authored-by: Philippe Rivière <[email protected]>

* Update docs/layout/grid.md

Co-authored-by: Philippe Rivière <[email protected]>

* remove overshare

---------

Co-authored-by: Philippe Rivière <[email protected]>
* add resize doc

* fix some typos

* fix formatting

* add both cases
@allisonhorst allisonhorst marked this pull request as ready for review January 18, 2024 19:28
@allisonhorst allisonhorst requested review from trebor and cinxmo January 18, 2024 19:31
@allisonhorst
Copy link
Contributor Author

Things I noticed during quick overall review, but did not update:

  • The 4x4 grid in layout/cards.md looks bad (spacing, not resizing)
  • Do we want consistent title conventions across sections? JavaScript section uses "JavaScript: Generators" convention, but in Inputs section we use "Button input" and in Charts we use "Area chart". Do we want update to "Inputs: Button", "Charts: Area" and similar throughout for naming consistency?
  • The link for formatLocaleAuto in Inputs API (https://github.com/observablehq/inputs?tab=readme-ov-file#formatLocaleAuto) doesn't take you anywhere on page (see in checkbox options). I updated to https://github.com/observablehq/inputs?tab=readme-ov-file#inputsformatlocaleautolocale. Is that correct?
  • The slug to view documentation (in javascript/inputs.md) has parentheses (#view(element)) which probably needs updating
  • Didn't move Top 10 bar chart, Weighted sum bar chart, or moving average line chart to the "Grouping data" page with hexbin & histogram, but it might make good sense to move them there since they use group/window transforms.
  • The "Grouping data" in the sidebar nav makes my eye twitch a bit alongside all the other single word Chart items

Observable Markdown offers a number of [built-in themes](./config#theme) that you can compose to create, say, wide pages with an alternative dark color theme:

```js run=false
theme: ["dark-alt", "wide"]
Copy link
Contributor

Choose a reason for hiding this comment

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

is dark-alt an official theme? or should it be ["dark", "alt", "wide"]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating to ["dark", "alt", "wide"] thanks!


```yaml
---
theme: ["dark-alt", "wide"]
Copy link
Contributor

@cinxmo cinxmo Jan 18, 2024

Choose a reason for hiding this comment

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

nit: I think both [dark-alt, wide] (without quotes) and ["dark-alt", "wide"] works. could we standardize between https://cli.observablehq.com/config#theme and here? also I see the pattern of having the description and then the code block:

The code below, when included in the [config file](./config), specifies the default theme for the project:

```js run=false
theme: ["dark-alt", "wide"]
```
In addition, you can specify the theme for a single page in its [front matter](markdown#front-matter):

```yaml
---
theme: ["dark-alt", "wide"]
---
```

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updating to match pattern in config (quotes in config file, no quotes in frontmatter)

Resize exists in the Observable standard library, or can be imported explicitly:

```js
import {resize} from "npm:@observablehq/stdlib";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we add this to show the explicit import?

import {resize} from "npm:@observablehq/stdlib";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, added echo

const pickSpecies = view(Inputs.radio(["Adelie", "Chinstrap", "Gentoo"], {value: "Gentoo", label: "Penguin species:"}))
```

The value of `pickSpecies` (<tt>="${pickSpecies}"</tt>) can then be accessed elsewhere in the page, as a parameter in other computations, and to create interactive charts, tables or text with [inline expressions](./javascript#inline-expressions).
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: seems like <tt> has been deprecated

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch, it should be <var>

@allisonhorst allisonhorst merged commit da9afa8 into main Jan 18, 2024
@allisonhorst allisonhorst deleted the components branch January 18, 2024 23:08
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.

5 participants