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

Fix timepicker button behavior in "No Results" page for timeseries data #6543

Merged
merged 9 commits into from
Jun 2, 2016

Conversation

bevacqua
Copy link
Contributor

Fixes #4947. Assigning @tsullivan because he initially reported the issue. Wasn't exactly sure where tests would go, please advise :)

@tsullivan
Copy link
Member

Sorry, I am not sure where tests should go. Perhaps @epixa or @spalger could advise.

@tsullivan
Copy link
Member

LGTM

@tsullivan tsullivan assigned bevacqua and unassigned tsullivan Mar 16, 2016
@bevacqua bevacqua assigned spalger and unassigned bevacqua Mar 16, 2016
@bevacqua
Copy link
Contributor Author

@spalger If you could take a look and let me know about any necessary tests that'd be awesome 🎉

@spalger
Copy link
Contributor

spalger commented Mar 18, 2016

Well, discover isn't well tested or easy to test right now, but if you move that button (or whole text) into a directive then you could test that (with kibana/public/discover/directives/no_results.js and kibana/public/discover/directives/__tests__/no_results.js).

Actually testing that clicking the button opens the timepicker is probably better done in a functional test though. So I would drop a couple data-test-subj attributes on the elements you want to test for, and then extend the functional discover tests to verify that clicking on the button does in fact toggle the timepicker.

@spalger spalger assigned bevacqua and unassigned spalger Mar 18, 2016
@bevacqua bevacqua removed the help wanted adoptme label Mar 18, 2016
@rashidkpc
Copy link
Contributor

@bevacqua where are we on this guy?

@bevacqua bevacqua force-pushed the bugfix/no-results-timepicker branch from 57edb37 to 348a939 Compare March 30, 2016 15:46
@bevacqua
Copy link
Contributor Author

Working on tests

@bevacqua bevacqua force-pushed the bugfix/no-results-timepicker branch from 6198feb to 4036c46 Compare March 30, 2016 17:09
@@ -0,0 +1,51 @@
<div ng-show="resultState === 'none'" data-test-subj="discoverNoResults">
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer if this ng-show was moved to the site where this directive is injected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

@spalger
Copy link
Contributor

spalger commented Mar 30, 2016

jenkins, test it

@bevacqua
Copy link
Contributor Author

bevacqua commented Apr 8, 2016

@spalger Build details are broken. Where can I look at the build output to learn what tests are failing here?

@spalger
Copy link
Contributor

spalger commented Apr 8, 2016

@bevacqua after a while builds are deleted. Just run the them again using a "jenkins, test it" comment.

@rashidkpc
Copy link
Contributor

jenkins, test it

@bevacqua
Copy link
Contributor Author

This is now fixed. @spalger up to re-review?

@spalger spalger assigned spalger and unassigned bevacqua May 24, 2016
@spalger
Copy link
Contributor

spalger commented May 24, 2016

Count me in!

@@ -241,6 +241,11 @@ import {
.catch(common.handleError(this));
});

bdd.it('should not show "no results"', () => {
discoverPage.hasNoResults().then(visible => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to return a promise

@spalger spalger assigned bevacqua and unassigned spalger May 24, 2016
@bevacqua
Copy link
Contributor Author

@spalger Returns all the promises now

@bevacqua bevacqua assigned spalger and bevacqua and unassigned bevacqua and spalger May 26, 2016
@bevacqua bevacqua assigned spalger and unassigned bevacqua May 31, 2016
@spalger
Copy link
Contributor

spalger commented Jun 2, 2016

LGTM

@spalger spalger assigned bevacqua and unassigned spalger Jun 2, 2016
@bevacqua bevacqua merged commit b6a0b89 into elastic:master Jun 2, 2016
@bevacqua
Copy link
Contributor Author

bevacqua commented Jun 2, 2016

Carry on 🎉

@bevacqua bevacqua removed their assignment Jun 2, 2016
@epixa epixa added v5.0.0 and removed v5.0.0 labels Jun 28, 2016
jbudz pushed a commit that referenced this pull request Jan 27, 2023
## Summary

`eui@73.0.0` ⏩ `eui@74.0.1`

---

## [`74.0.1`](https://github.com/elastic/eui/tree/v74.0.1)

**Bug fixes**

- Fixed `EuiModalHeaderTitle` type errors when passed `EuiTitle` props
([#6547](elastic/eui#6547))

## [`74.0.0`](https://github.com/elastic/eui/tree/v74.0.0)

- Added the `component` prop to `EuiModalHeaderTitle`, which allows
overriding the default `h1` tag
([#6530](elastic/eui#6530))
- Added the `titleProps` prop to `EuiConfirmModal`, which allows
overriding the default `h1` tag
([#6530](elastic/eui#6530))

**Bug fixes**

- Fixed slight row height jumping in `EuiBasicTable`s when actions with
tooltips became disabled
([#6538](elastic/eui#6538))

**Breaking changes**

- `EuiModalHeaderTitle` now **always** wraps its children in a `h1` tag
(previously attempted to conditionally detect whether its children were
raw strings or not). To change this tag type to, e.g. a more generic
`div`, use the new `component` prop.
([#6530](elastic/eui#6530))
- `EuiLink` now applies `rel="noreferrer"` to all domains, including
`elastic.co` ([#6535](elastic/eui#6535))
- `EuiBasicTable` no longer blocks mouse/keyboard interactions while
`loading` ([#6543](elastic/eui#6543))

**CSS-in-JS conversions**

- Converted `EuiBasicTable` to Emotion
([#6539](elastic/eui#6539))
- Added a new `RenderWithEuiTheme` render prop utility
([#6539](elastic/eui#6539))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kqualters-elastic pushed a commit to kqualters-elastic/kibana that referenced this pull request Feb 6, 2023
## Summary

`eui@73.0.0` ⏩ `eui@74.0.1`

---

## [`74.0.1`](https://github.com/elastic/eui/tree/v74.0.1)

**Bug fixes**

- Fixed `EuiModalHeaderTitle` type errors when passed `EuiTitle` props
([elastic#6547](elastic/eui#6547))

## [`74.0.0`](https://github.com/elastic/eui/tree/v74.0.0)

- Added the `component` prop to `EuiModalHeaderTitle`, which allows
overriding the default `h1` tag
([elastic#6530](elastic/eui#6530))
- Added the `titleProps` prop to `EuiConfirmModal`, which allows
overriding the default `h1` tag
([elastic#6530](elastic/eui#6530))

**Bug fixes**

- Fixed slight row height jumping in `EuiBasicTable`s when actions with
tooltips became disabled
([elastic#6538](elastic/eui#6538))

**Breaking changes**

- `EuiModalHeaderTitle` now **always** wraps its children in a `h1` tag
(previously attempted to conditionally detect whether its children were
raw strings or not). To change this tag type to, e.g. a more generic
`div`, use the new `component` prop.
([elastic#6530](elastic/eui#6530))
- `EuiLink` now applies `rel="noreferrer"` to all domains, including
`elastic.co` ([elastic#6535](elastic/eui#6535))
- `EuiBasicTable` no longer blocks mouse/keyboard interactions while
`loading` ([elastic#6543](elastic/eui#6543))

**CSS-in-JS conversions**

- Converted `EuiBasicTable` to Emotion
([elastic#6539](elastic/eui#6539))
- Added a new `RenderWithEuiTheme` render prop utility
([elastic#6539](elastic/eui#6539))

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants