Skip to content
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

Fixes #24129: fix subs. display when no manifest. #7516

Merged
merged 1 commit into from Jul 18, 2018

Conversation

waldenraines
Copy link
Contributor

@waldenraines waldenraines commented Jul 10, 2018

When there is no manifest:

  • Disable "Add subscription" button in the top right toolbar
    with an appropriate tooltip message.

  • Show different empty view on the subscriptions page saying
    "Import a Manifest to manage your Entitlements" or something
    similar and show "Manage Manifest" button.

  • Change the text in the add subscriptions empty view to the above.

https://projects.theforeman.org/issues/24129

TODO

  • Fix tests

@theforeman-bot
Copy link

Issues: #24129

Copy link
Contributor Author

@waldenraines waldenraines left a comment

Choose a reason for hiding this comment

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

Wanted to get some feedback on this before fixing the tests, let me know what you think.

@@ -186,6 +189,16 @@ class SubscriptionsPage extends Component {

const csvParams = createSubscriptionParams({ search: this.state.searchQuery });

const emptyStateData = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to pull this back into the subscriptions page component in order to open the manifest. Open to other solutions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tstrachota especially interested in your opinion on this since you did the initial refactor into the subscriptions table component.

Copy link
Member

Choose a reason for hiding this comment

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

I see two solutions here:

  1. The EmptyState component currently takes prop action: {url, title}. We can change it to {url, title, onClick} and let the component decide whether to wrap the button with LinkContainer (when url is present) or just render a button with onClick.
  2. Alternatively you could keep EmptyState as is in this PR and change the empty state date to a function and inject the handler:
const emptyStateData = ({onButtonClick}) => ({
  header: __('There are no Subscriptions to display'),
  description: __('Import a Manifest to manage your Entitlements.'),
  actionButton: (
    <Button onClick={onButtonClick}>
      {__('Import a Manifest')}
    </Button>
  ),
}};

Both solutions seem fine to me. I slightly tend to prefer the first one. Adding actionButton prop feels like adding another way to achieve the same to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tstrachota updated.

@waldenraines waldenraines changed the title [WIP] Fixes #24129: fix subs. display when no manifest. Fixes #24129: fix subs. display when no manifest. Jul 11, 2018
Copy link
Member

@tstrachota tstrachota left a comment

Choose a reason for hiding this comment

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

It works well @waldenraines!

I think that we should refactor the empty state as discussed in the other comment. Apart from that it's good.

@ehelms
Copy link
Member

ehelms commented Jul 16, 2018

[test katello/webpack]

Copy link
Contributor

@sharvit sharvit left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@tstrachota tstrachota left a comment

Choose a reason for hiding this comment

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

👍 works nicely

When there is no manifest:

- Disable "Add subscription" button in the top right toolbar
  with an appropriate tooltip message.

- Show different empty view on the subscriptions page saying
  "Import a Manifest to manage your Entitlements" or something
  similar and show "Manage Manifest" button.

- Change the text in the add subscriptions empty view to the above.

https://projects.theforeman.org/issues/24129
Copy link
Member

@jlsherrill jlsherrill left a comment

Choose a reason for hiding this comment

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

Approving based on other comments! Thanks @waldenraines

@waldenraines waldenraines merged commit d04bf68 into Katello:master Jul 18, 2018
@waldenraines waldenraines deleted the 24129 branch July 18, 2018 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants