-
Notifications
You must be signed in to change notification settings - Fork 841
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
[EUI+] Reorganize+improve markdown, tables, and data grid docs #8183
base: main
Are you sure you want to change the base?
Conversation
489e468
to
1372f28
Compare
1372f28
to
7930de3
Compare
+ several opinionated copy updates - move unregistering plugins to the plugins page, where it feels like it belongs significantly more
- create subpages, rewrite overview - move the table building blocks example to its own page (much better props DX) - rewrite a bunch of things that felt really hard to understand DX-wise - rewrite EuiBasicTable docs with in-memory table in mind (it felt - remove a bunch of manual pagination/sorting examples from docs - unnecessary extra complexity - improve smaller screen display by removing columns that didn't add much to DX use cases
+ split up container examples to their own subpage + clean up various headings/syntax
+ simplify grid row classes example further
- replace massive slow loading github JSON data with fake product data + simplify examples massively
- add a util for this with snippet generation
+ fix src-docs having the completely wrong example?? not sure what happened here, revert gone bad? - note that some examples won't work fully until package.json is pointed at latest EUI release or at workspace
…example to match prod - with new virtualization toggle/example - requires adding `raw-loader` dependency for separate sources/multiple files, but this is fairly minimal in the grand scheme of things and it's likely we may want it again in the future 🤷
0d2a303
to
bbbc124
Compare
- storybook needs this too, so might as well create a dedicated file for this
- switch to `Pick<>` over `Omit<>`, clearer typing - have to whitelist the component directly - we can't use `allowedParents` unfortunately because react-window's type name "CommonProps" is too generic :(
- this is due to all the components existing on the same page, which typically doesn't happen - we need to switch to a per-component array which doesn't get polluted as easily
bbbc124
to
0c35e41
Compare
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
id: editors_syntax_markdown_plugins | ||
--- | ||
|
||
# Markdown plugins | ||
|
||
Both **[EuiMarkdownEditor](/docs/editors-syntax/markdown-editor/)** and **[EuiMarkdownFormat](/docs/editors-syntax/markdown-format/)** utilize the same underlying plugin architecture to transform string based syntax into React components. At a high level [Unified JS(external, opens in a new tab or window)](https://www.npmjs.com/package/unified) is used in combination with [Remark(external, opens in a new tab or window)](https://www.npmjs.com/package/remark-parse) to provide EUI's markdown components, which are separated into a **parsing** and **processing** layer. These two concepts are kept distinct in EUI components to provide concrete locations for your plugins to be injected, be it editing or rendering. Finally you provide **UI** to the component to handle interactions with the editor. | ||
Both **[EuiMarkdownEditor](./editor)** and **[EuiMarkdownFormat](./format)** utilize the same underlying plugin architecture to transform string based syntax into React components. At a high level [Unified JS](https://www.npmjs.com/package/unified) is used in combination with [Remark](https://www.npmjs.com/package/remark-parse) to provide EUI's markdown components, which are separated into a **parsing** and **processing** layer. These two concepts are kept distinct in EUI components to provide concrete locations for your plugins to be injected, be it editing or rendering. Finally you provide **UI** to the component to handle interactions with the editor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this! I've been converting a few static /docs
URL to relative paths as I go, happy to do it here!
edit: ah wait hm, I got the relative pathing wrong on some of these. will fix
Tables can get complicated very fast. EUI provides both opinionated and non-opinionated ways to build tables. | ||
|
||
- **Opinionated high-level components:** | ||
- These high-level components removes the need to worry about constructing individual building blocks together. You simply arrange your data in the format it asks for. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit-pick:
- These high-level components removes the need to worry about constructing individual building blocks together. You simply arrange your data in the format it asks for. | |
- These high-level components remove the need to worry about constructing individual building blocks together. You simply arrange your data in the format it asks for. |
|
||
- **Opinionated high-level components:** | ||
- These high-level components removes the need to worry about constructing individual building blocks together. You simply arrange your data in the format it asks for. | ||
- [**EuiBasicTable**](./basic) handles mobile row selection, row actions, row expansion, and mobile UX out of the box with relatively simple-to-use APIs. It is best used with asynchronous data, or static datasets that do not need pagination/sorting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doubt:
The original note says:
If you're just looking for a basic table with pagination, sorting, checkbox selection, and actions then you should use EuiBasicTable
but we say:
It is best used with (...) static datasets that do not need pagination/sorting.
I'm a bit confused, I understand that the latter is correct? We should use EuiInMemoryTable
with pagination or separately add pagination to the EuiBasicTable
component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah the original note is just kind of not correct lol. I mean EuiBasicTable is fine if you want to manually slice your data for pagination, but I have no idea who would want to do that as it's a pain in the ass. It's basically:
- No complex pagination or sorting -> EuiBasicTable
- Server API is paginating/sorting for you -> use EuiBasicTable
- All your data is available and you want pagination/sorting handled automatically for you (the majority of use cases) -> EuiInMemoryTable
- You are a masochist and want to build your tables from scratch -> EuiTable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking suggestion:
I think we can remove the "In-memory table" from all the headers in the document.
--- | ||
|
||
# Data grid schema & columns | ||
# Schema & columns | ||
|
||
## Schemas | ||
|
||
Schemas are data types you pass to grid columns to affect how the columns and expansion popovers render. Schemas also allow you to define individual sorting comparators so that sorts can do more than just A-Z comparisons. By default, **EuiDataGrid** ships with a few built-in schemas for `numeric, currency, datetime, boolean and json` data. When the `inMemory` prop is in use it will automatically try to figure out the best schema based on the `inMemory:{{ level: value }}` you set, but this will come with the caveat that you will need to provide and manage sorting outside the component. In general we recommend passing schema information to your columns instead of using auto-detection when you have that knowledge of your data available during ingestion. | ||
|
||
### Defining custom schemas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking suggestion:
If we have a schema h2, how about we add h2 for columns? It'd make it a bit cleaner than having all these separate column-related h2's.
* The grid allows you to optionally define an [in memory level](./advanced#data-grid-in-memory) to have the grid automatically handle updating your columns. Depending upon the level chosen, you may need to manage the content order separately from the grid. | ||
* [Schemas](./schema-and-columns) allow you to tailor the render and sort methods for each column. The component ships with a few automatic schema detection and types, but you can also pass in custom ones. | ||
* Unlike tables, the data grid **forces truncation**. To display more content your can customize [popovers](./cells-and-popovers#conditionally-customizing-cell-popover-content) to display more content and actions into popovers. | ||
* [Grid styling](./style-display#grid-style) can be controlled by the engineer, but augmented by user preference depending upon the features you enable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking: This link doesn't work. Redirects to "NoSuchKey" error page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking suggestion:
There are 2 links that don't work here. If you end up fixing some stuff on this PR, it might be good to fix them as well. Otherwise, we can do it on: #8158 (which might have to be broken down but for now it's in one task).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking suggestion:
Again, not a big deal but if we moved all these to a separate folder data_grid
, we could also get rid of the prepended "data_grid" from all these files. It'll be neater ✨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, that would be super nice!
* Accepts any value that the `line-height` CSS property normally takes (e.g. px, ems, rems, or unitless) | ||
* `onChange` | ||
* Optional callback when the user changes the data grid's internal `rowHeightsOptions` (e.g., via the toolbar display selector). | ||
* Can be used to store and preserve user display preferences on page refresh - see this [data grid styling and control example](/docs/tabular-content/data-grid-style-display#adjusting-your-grid-to-usertoolbar-changes). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking suggestion:
I feel like this was just copied as is, so I consider it a non-blocker but this link is not working. We can fix it since we have the chance to do so. Otherwise, we can do it on #8158
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking suggestion:
I would move this to the /custom_grid_body
folder.
|
||
# Custom body rendering | ||
|
||
For **extremely** advanced use cases, the `renderCustomGridBody` prop may be used to take complete control over rendering the grid body. This may be useful for scenarios where the default [virtualized](/docs/tabular-content/data-grid#virtualization) rendering is not desired, or where custom row layouts (e.g., the conditional row details cell below) are required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking suggestion:
This link doesn't work. Feel free to fix it here or we'll do it separately on #8158
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking suggestion: I feel that instead of the nested structure, we could simplify and pull up the "Tables" and "Data grid", and get rid of the "Tabular content" category.
|
||
The following example demonstrates "actions" columns. These are special columns where you define per-row, item level actions. The most basic action you might define is a type `button` or `icon` though you can always make your own custom actions as well. | ||
Tables will be mobile-responsive by default, breaking down each row into its own card section and individually displaying each table header above the cell contents. The default breakpoint at which the table will responsively shift into cards is the [`m` window size](../../theming/breakpoints/values), which can be customized with the `responsiveBreakpoint` prop (e.g., `responsiveBreakpoint="s"`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking:
This link doesn't work. Feel free to fix here or let's do it later on #8158
@@ -159,7 +148,7 @@ import InMemoryTableSelection from './table_selection'; | |||
|
|||
## In-memory table with search | |||
|
|||
The example shows how to configure **EuiInMemoryTable** to display a search bar by passing the search prop. You can read more about the search bar's properties and its syntax [**here**](/docs/forms/search-bar) . | |||
This example shows how to configure **EuiInMemoryTable** to display a search bar by passing the `search` prop. For more detailed information about the syntax and configuration accepted by this prop, see [**EuiSearchBar**](../../forms/search-bar). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking:
This link doesn't work. Feel free to fix here or let's do it later on #8158
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking suggestion:
We could remove the prepending "markdown_" since it's already in the /markdown
folder. Just to make it squeaky clean.
id: editors_syntax_markdown_editor | ||
--- | ||
|
||
# Markdown editor | ||
|
||
**EuiMarkdownEditor** provides a markdown authoring experience for the user. The component consists of a toolbar, text area, and a drag-and-drop zone to accept files (if configured to do so). There are two modes: a textarea that keeps track of cursor position, and a rendered preview mode that is powered by **[EuiMarkdownFormat](/docs/editors-syntax/markdown-format/)**. State is maintained between the two and it is possible to pass changes from the preview area to the textarea and vice versa. | ||
**EuiMarkdownEditor** provides a markdown authoring experience for the user. The component consists of a toolbar, text area, and a drag-and-drop zone to accept files (if configured to do so). There are two modes: a textarea that keeps track of cursor position, and a rendered preview mode that is powered by **[EuiMarkdownFormat](./format)**. State is maintained between the two and it is possible to pass changes from the preview area to the textarea and vice versa. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking:
These links don't work. Feel free to fix here or let's do it later on #8158
--- | ||
|
||
# Markdown format | ||
|
||
**EuiMarkdownFormat** is a read-only way to render markdown-style content in a page. It is a peer component to **[EuiMarkdownEditor](/docs/editors-syntax/markdown-editor/)** and has the ability to be modified by additional [markdown plugins](/docs/editors-syntax/markdown-plugins). | ||
**EuiMarkdownFormat** is a read-only way to render markdown-style content in a page. It is a peer component to **[EuiMarkdownEditor](./editor)** and has the ability to be modified by additional [markdown plugins](./plugins). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking:
These links don't work. Feel free to fix here or let's do it later on #8158
Incredible changes, Cee! 🥳 🎉 Both of these categories felt messy and unintuitive, now it's easy to navigate around and the information is more granular. I still went over the code changes cause they're quite significant. I didn't find anything that would be a blocker for this PR though. I agree that we can improve later and it'd feel wasteful for you to refactor this right now. I tested the staging: ✅ no pages are missing If you're busy with other things, I can push the changes to the PR and we'll have the suggestions sorted out already instead of leaving them for later. Let me know what you prefer 😄 |
Commenting just to wrap this up/for extra redundancy - please feel free to push up any changes to this PR and get it merged in. The suggestions you made look great! |
Summary
#8163
I apologize in advance for how large this PR/diff is - this PR really got away from me, for which I wholly blame datagrid 🥲
Honestly, I would probably suggest skipping code review for this one and just reviewing the end-user docs directly compared to previous iterations.
I'm definitely looking for feedback on anything that's obviously broken in terms of functionality, or any copy/organization that feels straight up not-understandable, but as a heads up I'm likely not going to want a whole ton of large refactoring on this PR since I don't have much more time at Elastic - I'd like to get this in as before too long (given that it doesn't flagrantly break anything in prod) and continue to iterate in the future as needed.
QA
General checklist
N/A, docs only