Skip to content

Link XYChart components to experimental directory#975

Merged
markov00 merged 3 commits into
elastic:masterfrom
markov00:move-xychart-experimental
Jul 9, 2018
Merged

Link XYChart components to experimental directory#975
markov00 merged 3 commits into
elastic:masterfrom
markov00:move-xychart-experimental

Conversation

@markov00
Copy link
Copy Markdown
Contributor

@markov00 markov00 commented Jul 9, 2018

This PR comes after a discussion with @cchaos @snide @mattapperson about specifying a bit more that these is a beta/experimental version of the XYChart and that APIs and features can change easily in the feature.

This PR changed the way to import beta XYChart components, forcing developers to import these components from /experimental path like:

import { EuiXYChart } from '@elastic/eui/experimental';

The components are not moved from the /components folder, but the exporting module reside in the /src/experimental/index.js file.

The PR also updated the function for rendering example code to correctly handle the experimental components.

Copy link
Copy Markdown
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

LGTM, all the demos still load properly. Just one nit-picky comment and don't forget to add a changelog entry.

Comment thread src/index.js Outdated
export * from './components';
export * from './services';
export * from './utils';
export * from './experimental';
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.

Can you re-order this list to be alphabetical order?

Copy link
Copy Markdown
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Nice work! LGTM.

.replace(/(from )'(..\/)+src\/components(\/?';)/, `from '@elastic/eui';`)
.replace(/(from )'(..\/)+src\/services(\/?';)/, `from '@elastic/eui/services';`);
.replace(/(from )'(..\/)+src\/services(\/?';)/, `from '@elastic/eui/services';`)
.replace(/(from )'(..\/)+src\/experimental(\/?';)/, `from '@elastic/eui/experimental';`);
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.

❤️

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.

@cjcenizal could you check this? #976 because I'm not really sure if it's better to have /lib/experimental or /experimental

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.

and maybe it's related to the build

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.

I think the correct path would be eui/lib/experimental because otherwise someone who copy-pastes the code would have a broken import, right?

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.

I think so, also for services needs to be the same. I will change it and than merge it.

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.

Thanks, sounds good!

Copy link
Copy Markdown
Contributor

@chandlerprall chandlerprall Jul 9, 2018

Choose a reason for hiding this comment

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

CJ is correct, and the existing @elastic/eui/services is wrong and should be @elastic/eui/lib/services

@markov00 markov00 merged commit 143e19e into elastic:master Jul 9, 2018
@markov00 markov00 deleted the move-xychart-experimental branch July 9, 2018 17:34
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.

4 participants