React components for ManageIQ textual summary

[martinpovolny]

This is a continuation of ManageIQ/manageiq-ui-classic#3420

• I am moving the components to the components repo, trying to decouple the specifics.
• Adding more specs,
• Adding simple storybook

[ohadlevy]

might make sense to bump to 2.x as it already introduced api changes (for something we dont use yet - but can't see the value of using an older version now).

[martinpovolny]

I need this compatible with ui-classic. The goal is to replace TextualSummaries in ui-classic. UI-classic is on 1.X.

[ohadlevy]

should these strings be extracted? or are they used as props somewhere else?

[martinpovolny]

This is test data. It is used in stories and tests.

[ohadlevy]

How about using patternfly tables? https://rawgit.com/patternfly/patternfly-react/gh-pages/index.html?selectedKind=Table&selectedStory=PatternFly%20Table%20Styles&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybooks%2Fstorybook-addon-knobs

[martinpovolny]

I can do it, but not in the first implementation. My goal in this PR is to create React components from HAML files.

[ohadlevy]

my main concern is that it things that are kept for next version are often skipped, also even if you are not using the pf table implementation, I do think you need a table component instead of using html.

…

On Thu, May 3, 2018 at 11:51 AM, Martin Povolny ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/textual_summary/empty_group.jsx <#14 (comment)> : > @@ -0,0 +1,24 @@ +import * as React from 'react'; +import PropTypes from 'prop-types'; + +export default function EmptyGroup(props) { + return ( + <table className="table table-bordered table-striped table-summary-screen"> I can do it, but not in the first implementation. My goal in this PR is to create React components from HAML files. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#14 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABOx30q8aNfEGmbkhNbwChsNpLBqlPWks5tusUUgaJpZM4Twnxu> .

[martinpovolny]

1. I don't think that this is a problem I have.
2. I have other things that I need to do now.
3. Size (scope) of a PR is to be limited. A step at a time. I am already splitting the original PR into 4 parts. I don't feel I should be adding more to the scope now.

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[karelhala]

Maybe rewriting this to hash might be better than using switch

const componentMapper = {
  GenericGroup: props => (
    <GenericGroup onClick={props.onClick} items={props.group.items} title={props.group.title} />
  ),
  TagGroup: props => <TagGroup items={props.group.items} title={props.group.title} />,
  SimpleTable: props => (
    <SimpleTable
      labels={props.group.labels}
      rows={props.group.rows}
      title={props.group.title}
    />
  ),
  OperationRanges: props => <OperationRanges items={props.group.items} title={props.group.title} />,
  MultilinkTable: props => (
    <MultilinkTable
      onClick={props.onClick}
      items={props.group.items}
      title={props.group.title}
    />
  ),
  TableListView: props => (
    <TableListView
      rowLabel={props.group.rowLabel}
      onClick={props.onClick}
      title={props.group.title}
      headers={props.group.headers}
      values={props.group.values}
      colOrder={props.group.colOrder}
    />
  ),
  EmptyGroup: props => <EmptyGroup title={props.group.title} text={props.group.text} />
};

That way you can specify which component is using which propTypes

componentMapper.GenericGroup.propTypes = {
  group: PropTypes.shape({
    title: PropTypes.string.isRequired,
    items: PropTypes.any,
  }).isRequired,
  onClick: PropTypes.func.isRequired,
};

componentMapper.TagGroup.propTypes = {
  group: PropTypes.shape({
    title: PropTypes.string.isRequired,
    items: PropTypes.any,
  }).isRequired,
};

Or use spread operator and inherit these properties.

[martinpovolny]

Maybe rewriting this to hash might be better than using switch

If you mean a simple hash, that it would not work, because a hash is not lazy evaluated. All the branches would get called and then the apropriate result would be selected. So that approach would crash on the propTypes mismatch (different case branches need diffferent properties).

[martinpovolny]

Oh, you mean a hash of functions. Then yes, it would work but is a hash of functions actually better than what I have now?

[martinpovolny]

Anyway. If it's a 'maybe changing it will look better' then I am not changing it unless you insist. I see the difference as marginal.

[miq-bot]

Checked commits martinpovolny/react-ui-components@a143eb5~...2b22d5f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🍪