Skip to content

[Docs] Fix all EuiBasicTable and EuiInMemoryTable demos to work in CodeSandbox#6553

Merged
cee-chen merged 11 commits intoelastic:mainfrom
cee-chen:docs/table-custom
Jan 26, 2023
Merged

[Docs] Fix all EuiBasicTable and EuiInMemoryTable demos to work in CodeSandbox#6553
cee-chen merged 11 commits intoelastic:mainfrom
cee-chen:docs/table-custom

Conversation

@cee-chen
Copy link
Copy Markdown
Contributor

@cee-chen cee-chen commented Jan 26, 2023

Summary

closes #5959

This PR converts the last EuiBasicTable demo and all EuiInMemoryTable demos to Typescript, removes the data_store dependency, and deletes the data_store file.

QA

General checklist

@cee-chen cee-chen added documentation Issues or PRs that only affect documentation - will not need changelog entries skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) labels Jan 26, 2023
@cee-chen cee-chen requested a review from elizabetdev January 26, 2023 03:42
@kibanamachine
Copy link
Copy Markdown

Preview documentation changes for this PR: https://eui.elastic.co/pr_6553/

@kibanamachine
Copy link
Copy Markdown

Preview documentation changes for this PR: https://eui.elastic.co/pr_6553/

Copy link
Copy Markdown
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Thanks, @cee-chen. I tested all examples and LGTM.

Just found one eslint warning.

And because the data_store.js file was deleted I searched for data_store and found some instances on the following page:

import { data } from '../data_store';

Is this something we need to delete?

const [message, setMessage] = useState(
const [users, setUsers] = useState<User[]>([]);
const [message, setMessage] = useState<ReactNode>(
<EuiEmptyPrompt
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On line 145 in CodeSandbox there's an eslint warning for this file:

'loadUsers' was used before it was defined. (@typescript-eslint/no-use-before-define)eslint

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That issue was already "there" pre-typescript if that makes sense, but because the file wasn't .tsx, that particular eslint rule wasn't coming up 🤷 TBH it doesn't bother me and hoisting is legit JS, but I can take a look at trying to get rid of it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ha this is pretty annoying, loadUsers calls setMessage so there isn't a super quick or clean way to separate the two. I'm probably just going to ignore this TBH, it still works fine and our own codebase doesn't lint for this rule, so I'm pretty eh about it

Copy link
Copy Markdown
Contributor Author

@cee-chen cee-chen Jan 26, 2023

Choose a reason for hiding this comment

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

Also to answer:

Is this something we need to delete?

Nope, that particular file / test usage is a static string and not an actual import (the file is testing a regex).

@cee-chen cee-chen merged commit 2a35923 into elastic:main Jan 26, 2023
@cee-chen cee-chen deleted the docs/table-custom branch January 26, 2023 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Issues or PRs that only affect documentation - will not need changelog entries skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Line 35 isFunction is missing in demo code.

3 participants