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

[APM] UI filters: Change transaction type selector from dropdown to radio buttons #75625

Merged
merged 7 commits into from
Aug 25, 2020
Merged

[APM] UI filters: Change transaction type selector from dropdown to radio buttons #75625

merged 7 commits into from
Aug 25, 2020

Conversation

cauemarcondes
Copy link
Contributor

closes #75427

Screenshot 2020-08-21 at 10 32 10

Screenshot 2020-08-21 at 10 32 19

@cauemarcondes cauemarcondes requested a review from a team as a code owner August 21, 2020 08:33
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Aug 21, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

formgeist
formgeist approved these changes Aug 21, 2020
View reviewed changes
Copy link
Contributor

@formgeist formgeist left a comment

Choose a reason for hiding this comment

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

LGTM - minor style sugggestion improvement to the filters, so separate each section a bit more. I noticed that we have an EuiSpacer component between each value component. I'd like to suggest we change the prop from s to m on the bottom spacer. Like so;

Screenshot 2020-08-21 at 10 53 38

@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

@formgeist
Copy link
Contributor

@cauemarcondes I noticed one thing (not sure it has anything to do with this specific PR) where the EuiBadge with the close icon doesn't show a link cursor, that I'd expect.

Kapture 2020-08-24 at 9 58 43

Shall I open another bug report?

@cauemarcondes
Copy link
Contributor Author

@cauemarcondes I noticed one thing (not sure it has anything to do with this specific PR) where the EuiBadge with the close icon doesn't show a link cursor, that I'd expect.

Kapture 2020-08-24 at 9 58 43

Shall I open another bug report?

It's not related, but I'll take a look and fix it in this PR too.

@cauemarcondes
Copy link
Contributor Author

cauemarcondes commented Aug 24, 2020 •
edited
Loading

@formgeist 8e113d1, fix to add the link cursor to the badges

@formgeist
Copy link
Contributor

@formgeist 8e113d1, fix to add the link cursor to the badges

Looks good 👍

smith
smith requested changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@smith smith 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. Requesting changes to add i18n for the ARIA labels.

.../plugins/apm/public/components/app/TransactionOverview/__jest__/TransactionOverview.test.tsx Outdated Show resolved Hide resolved
.../plugins/apm/public/components/app/TransactionOverview/__jest__/TransactionOverview.test.tsx Outdated Show resolved Hide resolved
@cauemarcondes cauemarcondes requested a review from smith August 25, 2020 07:16
sorenlouv
sorenlouv approved these changes Aug 25, 2020
View reviewed changes
Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

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

lgtm

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
apm 4.7MB +246.0B 4.7MB

History

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

@cauemarcondes cauemarcondes merged commit 1e8c05f into elastic:master Aug 25, 2020
@cauemarcondes cauemarcondes deleted the apm-transaction-filter branch August 25, 2020 14:15
kibanamachine pushed a commit that referenced this pull request Aug 25, 2020
@cauemarcondes @elasticmachine
[APM] UI filters: Change transaction type selector from dropdown to r…
8f22849 
…adio buttons (#75625)

* changing transaction type filter to radio group

* fixing unit test

* changing transaction type filter to radio group

* adding onclick to the badge component

* adding onclick to the badge component

* adding i18n to aria

Co-authored-by: Elastic Machine <[email protected]>
@kibanamachine kibanamachine mentioned this pull request Aug 25, 2020
Merged
@kibanamachine
Copy link
Contributor

💚 Backport successful

The PR was backported to the following branches:

cauemarcondes added a commit that referenced this pull request Aug 25, 2020
@kibanamachine @cauemarcondes @elasticmachine
[APM] UI filters: Change transaction type selector from dropdown to r…
f39cbbb 
…adio buttons (#75625) (#75877)

* changing transaction type filter to radio group

* fixing unit test

* changing transaction type filter to radio group

* adding onclick to the badge component

* adding onclick to the badge component

* adding i18n to aria

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Cauê Marcondes <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@formgeist formgeist formgeist approved these changes

@sorenlouv sorenlouv sorenlouv approved these changes

@smith smith smith approved these changes

Assignees
No one assigned
Labels
release_note:enhancement Team:APM All issues that need APM UI Team support v7.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] UI filters: Change transaction type selector from dropdown to radio buttons
6 participants
@cauemarcondes @elasticmachine @formgeist @kibanamachine @smith @sorenlouv