Skip to content

Conversation

@amarie401
Copy link
Contributor

Description

Ability for pfEmptyState component config to have url action to provide a user defined callback method.

Specifically this will be helpful for this issue:
openshift/origin-web-catalog#461

Which would allow the clear applied filters function to be applied to the url link when a user clicks the link on the empty state. Currently only a url can be provided.

Rawgit

PR Checklist

  • Unit tests are included

expect(element.find('.blank-state-pf-helpLink').text()).toContain('For more information please see');
expect(element.find('a').text()).toContain('pfExample');
expect(element.find('a').prop('href')).toContain('#/api/patternfly.views.component:pfEmptyState');
expect(element.find('.blank-state-pf-helpLink').click().length).toBe(1);
Copy link
Member

Choose a reason for hiding this comment

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

I think a better way to test callback functions in unit tests would be:

element.find('.blank-state-pf-helpLink').click();
expect($scope.eventText).toBe('Empty State Action Executed');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha! I had something similar to that initially, thanks Dave! :)

* <li>.info - (string) Text for the main informational paragraph
* <li>.helpLink - (object) Contains url specific properties and actions
* <ul style='list-style-type: none'>
* <li>.label - (string) Text for label
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps "Text label which appears before the urlLabel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good!

* <li>.helpLink - (object) Contains url specific properties and actions
* <ul style='list-style-type: none'>
* <li>.label - (string) Text for label
* <li>.urlLabel - (string) Text for url label
Copy link
Member

@dtaylor113 dtaylor113 Oct 13, 2017

Choose a reason for hiding this comment

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

How about: "Text for the clickable portion of the link"

Copy link
Member

Choose a reason for hiding this comment

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

Still waiting on this minor fix. After these textual changes, everything LGTM.

@amarie401 amarie401 force-pushed the emptystate-filter branch 2 times, most recently from 0e55ffd to 241de6f Compare October 13, 2017 15:45
* <ul style='list-style-type: none'>
* <li>.label - (string) Text label which appears before the
* <li>.urlLabel - (string) Text for the clickable portion of the link
* <li>.url - (string) Text for url path
Copy link
Member

Choose a reason for hiding this comment

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

This is optional, right?

Copy link
Contributor Author

@amarie401 amarie401 Oct 13, 2017

Choose a reason for hiding this comment

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

Yes this is optional.

* <li>.label - (string) Text label which appears before the
* <li>.urlLabel - (string) Text for the clickable portion of the link
* <li>.url - (string) Text for url path
* <li>.urlAction - (function) Function to invoke a url action when a callback method is specified.
Copy link
Member

Choose a reason for hiding this comment

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

This is optional, right?

Copy link
Contributor Author

@amarie401 amarie401 Oct 13, 2017

Choose a reason for hiding this comment

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

Yes this is optional. Will make sure to note that.

Copy link
Contributor Author

@amarie401 amarie401 Oct 13, 2017

Choose a reason for hiding this comment

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

helpLink and it's properties are under the "config Optional configuration object" for the config param in ngDocs. Should I still note that it's optional? @jeff-phillips-18

Copy link
Member

Choose a reason for hiding this comment

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

I would still mark them as optional for that object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay will do.

if (ctrl.config.helpLink.urlAction) {
ctrl.config.helpLink.urlAction();
}
};
Copy link
Member

Choose a reason for hiding this comment

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

This is unnecessary. You can just use ng-click="$ctrl.config.helpLink.urlAction()", if it is not defined nothing will happen, no error.

@amarie401 amarie401 force-pushed the emptystate-filter branch 4 times, most recently from ee4d4cf to 98b6f0e Compare October 13, 2017 19:30
* <li>.helpLink - (object) Contains url specific properties and actions
* <ul style='list-style-type: none'>
* <li>.label - (string) Optional text label which appears before the
* <li>.urlLabel - (string) Optional text for the clickable portion of the link
Copy link
Member

Choose a reason for hiding this comment

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

How about "...before the urlLabel"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops that would be helpful huh? :)

Copy link
Member

@cdcabrera cdcabrera left a comment

Choose a reason for hiding this comment

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

Looks good to me, simple and to the point!

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