Skip to content

Conversation

@clintandrewhall
Copy link
Contributor

This is a PR abstracted from #69357 and is a pre-req for the functionality it provides.

Summary

I'm proposing a new structure for our components that is demonstrated here with asset and asset_manager:

- components
  - foo                       <- directory
    - index.ts                <- thin exporting index, (no redux)
    - foo.ts                  <- redux container
    - foo.component.tsx       <- react component
  - bar
    - index.ts                <- thin exporting index, (no redux)
    - bar.ts                  <- redux container
    - bar.component.tsx       <- react component, consumes sub-component container
    - bar_dep.ts              <- redux sub-component container
    - bar_dep.component.tsx   <- sub-component

The root exporting file would then no longer by a redux container, but rather a thin index:

export { Foo } from './foo';
export { Foo as FooComponent } from './foo.component';

Notes

  • I'm excluding from Storyshots testing any Storybook stories whose titles start with redux. This is to avoid storyshot bloat, (as Redux-enabled stories could likely include much more markup, and, as they are dynamic in nature, a shot of the beginning structure is not as helpful).
  • This PR includes a rudimentary Redux Provider for the Asset examples. This is temporary, as [Canvas][PoC] Add Redux Inspector to Storybook; standardize names and convert a few stories #69357 provides a much more robust solution and will be in a follow-on PR.

@clintandrewhall clintandrewhall requested a review from a team as a code owner July 19, 2020 17:46
@clintandrewhall clintandrewhall added impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v7.10.0 v8.0.0 labels Jul 19, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas (Team:Canvas)

Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

I really like this setup! Feels easier to get at either the redux wrapped or non-redux components. And heck yes to having the contributing guidelines!

@clintandrewhall
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
canvas 1.5MB -772.0B 1.5MB

page load bundle size

id value diff baseline
canvas 1.4MB +153.0B 1.4MB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@clintandrewhall clintandrewhall merged commit a4957e6 into elastic:master Jul 21, 2020
clintandrewhall added a commit to clintandrewhall/kibana that referenced this pull request Jul 22, 2020
* New Component Layout proposal

* Add contribution guidelines; remove dead i18n

* Re-adding i18n... ugh

* Fix i18n files to reflect changes

* Addressing feedback

* Fix merge issue

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 22, 2020
* master: (23 commits)
  Stabilize closing toast (elastic#72097)
  stabilize failing test (elastic#72086)
  Stabilize filter bar test (elastic#72032)
  Unskip vislib tests (elastic#71452)
  [ML] Fix layout of anomaly chart tooltip for long field values (elastic#72689)
  fix preAuth/preRouting mocks (elastic#72663)
  [Security Solution] Hide KQL bar (all pages) and alerts filters (Detections) when Resolver is full screen (elastic#72788)
  [Uptime] Rename Whitelist to Allowlist in parse_filter_map (elastic#71584)
  [Security Solution] Fixes exception modal not loading content (elastic#72770)
  [Security Solution][Exceptions] - Require non empty entries and non empty string values in exception list items (elastic#72748)
  [Detections] Add validation for Threshold value field (elastic#72611)
  [SIEM][Detection Engine][Lists] Adds version and immutability data structures (elastic#72730)
  [Security Solution][Detections] Validate file type of value lists (elastic#72746)
  [pre-req] New Component Layout proposal (elastic#72385)
  [ML] do not throw an error when agg is not supported by UI (elastic#72685)
  [Resolver] Origin process (elastic#72382)
  [Ingest Manager] Allow to force unenroll from the UI (elastic#72386)
  skip 6.8 branch when triggering baseline-capture builds (elastic#72706)
  [CI] In-progress PR comments (elastic#72211)
  Fix sorting of scripted string fields (elastic#72681)
  ...
clintandrewhall added a commit that referenced this pull request Jul 22, 2020
* New Component Layout proposal

* Add contribution guidelines; remove dead i18n

* Re-adding i18n... ugh

* Fix i18n files to reflect changes

* Addressing feedback

* Fix merge issue

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@clintandrewhall clintandrewhall deleted the component-structure branch July 9, 2021 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas t// v7.10.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants