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.

Sign up for GitHub

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

Jump to bottom

[Uptime] Ping List Disable expand row if no body present #54898

Merged
merged 10 commits into from
Jan 22, 2020
Merged

[Uptime] Ping List Disable expand row if no body present #54898

merged 10 commits into from
Jan 22, 2020

Conversation

shahzad31
Copy link
Contributor

Summary

Fix: elastic/uptime#125

This PR will disable expand row arrow if no content is available for expand row to display.
image

Also modified to display error in EuiCallout Error state and body in default EuiCallout

For Error State:
image

For Body State:
image

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@shahzad31 shahzad31 requested review from justinkambic and andrewvc and removed request for justinkambic January 15, 2020 11:52
@shahzad31 shahzad31 self-assigned this Jan 15, 2020
@shahzad31 shahzad31 added release_note:fix Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.6.0 v8.0.0 labels Jan 15, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@shahzad31
Copy link
Contributor Author

@andrewvc should we be displaying body, if there is no content in the body but bytes size is available?

@andrewvc
Copy link
Contributor

@shahzad31 we should display the byte count, and probably a message saying :

Body not recorded. Read our docs for more information on recording response bodies

@andrewvc
Copy link
Contributor

@katrin-freihofner mind chiming in ^^^ with any thoughts on this approach?

@katrin-freihofner
Copy link
Contributor

@shahzad31 we should display the byte count, and probably a message saying :

Body not recorded. Read our docs for more information on recording response bodies

Yes, I think this is a great idea. Let's give the user all the information we have and explain why there is no body.

What do you think about adding a title that says something like "No details available".

@shahzad31
Copy link
Contributor Author

@katrin-freihofner mind chiming in ^^^ with any thoughts on this approach?

@andrewvc have added that

image

andrewvc
andrewvc requested changes Jan 20, 2020
View reviewed changes
Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

Functionality is great in manual testing and code looks good aside from the comments I left.

Can you also rename this PR and update the description to reflect what work was actually done?

x-pack/legacy/plugins/uptime/public/components/functional/ping_list/doc_link_body.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/uptime/public/components/functional/ping_list/ping_list.tsx Show resolved Hide resolved
x-pack/legacy/plugins/uptime/public/components/functional/ping_list/expanded_row.tsx
<BodyExcerpt content={body.content || ''} />
</Fragment>
<EuiSpacer size={'s'} />
{body.content ? <BodyExcerpt content={body.content || ''} /> : <DocLinkForBody />}
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT we don't have a test case for this. The snapshot was updated, but there's no case where I see this rendered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the case for this in test file.

@shahzad31
Copy link
Contributor Author

@andrewvc Thanks for the review, i have taken care of all the feedback.

andrewvc
andrewvc approved these changes Jan 22, 2020
View reviewed changes
Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

LGTM

@shahzad31
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

user doesn't have permission to update head repository

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@shahzad31 shahzad31 merged commit c6f9eff into elastic:master Jan 22, 2020
@shahzad31 shahzad31 deleted the fix/ping-list-row branch January 22, 2020 09:55
This was referenced Jan 23, 2020
Merged
Merged
shahzad31 added a commit to shahzad31/kibana that referenced this pull request Jan 23, 2020
@shahzad31
[Uptime] Ping List Disable expand row if no body present (elastic#54898)
2253728 
* update ping list

* update snap

* updated body

* update snaps

* fix i18n

* updated translation

* updated tests
shahzad31 added a commit that referenced this pull request Jan 23, 2020
@shahzad31
[Uptime] Ping List Disable expand row if no body present (#54898) (#5…
071e41a 
…5724)

* update ping list

* update snap

* updated body

* update snaps

* fix i18n

* updated translation

* updated tests
shahzad31 added a commit that referenced this pull request Jan 23, 2020
@shahzad31
[Uptime] Ping List Disable expand row if no body present (#54898) (#5…
0fb7ad7 
…5723)

* update ping list

* update snap

* updated body

* update snaps

* fix i18n

* updated translation

* updated tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewvc andrewvc andrewvc approved these changes

@justinkambic justinkambic Awaiting requested review from justinkambic

Assignees

@shahzad31 shahzad31

Labels
release_note:fix Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.6.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Should PingList expandable row be disabled if there is no content to show?
5 participants
@shahzad31 @elasticmachine @andrewvc @katrin-freihofner @kibanamachine