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

Stack trace local variables styling euitable #47128

Closed
wants to merge 9 commits into from

Conversation

@ffknob
Copy link
Contributor

commented Oct 2, 2019

Summary

Changed the 'local variables' style in Stacktrace to use Eui, and not anymore the inline styling.
It looks a little different now, I choose to get rid of the "VariablesToggle" and use EuiAccordion instead.

Before:

local-variables-before

After:

local-variables-after

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2019

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@@ -3,7 +3,7 @@
"data_autocomplete_rules": {
"metadata": {},
"password": "",
"fullname": "",
"full_name": "",

This comment has been minimized.

Copy link
@sqren

sqren Oct 2, 2019

Member

Why is this changed?

@@ -3,7 +3,7 @@
"data_autocomplete_rules": {
"metadata": {},
"password": "",
"fullname": "",
"full_name": "",

This comment has been minimized.

Copy link
@sqren

sqren Oct 2, 2019

Member

Same with this.

This comment has been minimized.

Copy link
@ffknob

ffknob Oct 2, 2019

Author Contributor

I guess this comes from another PR I did a while ago: #37033

Sorry, I am new to this whole Git flow process... I'm not sure why this came together with the modifications I made for this particular issue.

This comment has been minimized.

Copy link
@sqren

sqren Oct 2, 2019

Member

No worries :)

<EuiSpacer />
<EuiAccordion
id="local-variables"
buttonContent={this.localVariablesToogleButtonLabel()}

This comment has been minimized.

Copy link
@sqren

sqren Oct 2, 2019

Member

This doesn't need to be a separate function

Suggested change
buttonContent={this.localVariablesToogleButtonLabel()}
buttonContent={i18n.translate('xpack.apm.stacktraceTab.localVariablesToogleButtonLabel', { defaultMessage: 'Local variables' }) }
@sqren
sqren approved these changes Oct 2, 2019
Copy link
Member

left a comment

Thanks for the contribution. Code changes LGTM. Can you add a before / after screenshot to the description?

@formgeist what do you think of the new look?

@sqren

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

@ffknob Will you remove the unrelated files, and then this is good to go.

@formgeist

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

@ffknob Thanks for taking this on 👍 I have some minor visual feedback on how the section is presented in the stackframe panel.

Would it be possible to move the accordion collapsed/expand button outside the stackframe background? And could we add some margins around the table itself too? Here's a screen that kind of shows what I'm looking for;

apm-stackframe-local-vars-collapsed
apm-stackframe-local-vars-expaned

@ffknob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

Hi guys, here's what've done...

  1. 'Local variables' label defined by inline code, instead of a function
  2. I think I managed to bring the other two unrelated files back to the originals (I just brought the content back to what it looks like in master... altought I still am not sure how they got into my commit... and also how is this change not part of master, since I think this PR I did a while ago was merged... anyway...)
  3. Gave a shot to bring the look&feel more to what @formgeist asked for (yeah, that was bothering me too). I now use a EuHorizontalRule instead of EuiSpacer, but I had to bring back some of the old inline css code (and its imports)... Collapsed/Expanded image below

Collapsed
collapsed

Expanded
expanded

Updated

  • Applied "euiAccordion" class to the component and now it looks even better (spacing)
  • But I don't know why the link color did not get to "#006BB4" (@elastic/eui/dist/eui_theme_light.css:338)
Copy link
Contributor

left a comment

Just a minor nit on the container adding its own font-family.

@formgeist

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

But I don't know why the link color did not get to "#006BB4" (@elastic/eui/dist/eui_theme_light.css:338)

I think it's because the default styling of the accordion toggle button is not the Kibana blue but instead the default text colour. I don't think that's too critical in this case. We can probably find a way to duplicate the button style from the library frames toggle.

Screenshot 2019-10-08 at 14 55 48

Copy link
Contributor

left a comment

Just another visual nit 🙂

@ffknob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

Just removed both font-family and horizontal rule.

@sqren

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

@elasticmachine merge upstream

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

merge conflict between base and head

@sqren

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

@ffknob Can you pull in master and fix the conflict?
(there is a conflict in x-pack/plugins/apm/public/components/shared/Stacktrace/Variables.tsx)

ffknob added 6 commits Oct 2, 2019
… anymore the inline styling.

It looks a little different now, I choose to get rid of the "VariablesToggle" and use EuiAccordion instead.
@ffknob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

I found an article with instructions and then did a local hard reset last commit before breaking everything)

git reset --hard 2a9141a

It then says to --force push... Should I?
This time I prefer to check with you guys for instructions... All I want is to go back to my only 1 file commit

Once again sorry for the git-noobness and thanks for the help

@sqren

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

Hey @ffknob

no worries, that happens. Without looking more into this I don't know what the best approach to revert is. You are welcome to try force pushing 👍

@ffknob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

Hey @ffknob

no worries, that happens. Without looking more into this I don't know what the best approach to revert is. You are welcome to try force pushing

Back to 1 file changed.
Now I'll do what I should've done in the first place:

Can you pull in master and fix the conflict?

How can I verify this conflict?
I guess it will show as a result of the "kibana-ci Expected — Waiting for status to be reported", right?

And now that I managed to bring back to my 1 file changed commit, could you please guide me with the "pull in master" part? I guess not the fetch/rebase/pull (some of that I took from the CONTRIBUTING guide)

@sqren

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

retest

@sqren

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

How can I verify this conflict?

Looks like you fixed the conflict already. 👍 I've kicked off a build and will merge if it's successful. Thanks for the contribution

@ffknob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2019

 9 import { EuiHorizontalRule, EuiAccordion } from '@elastic/eui';
            ~~~~~~~~~~~~~~~~~
 x-pack/legacy/plugins/apm/public/components/shared/Stacktrace/Variables.tsx:14:3 - error TS6133: 'fontFamily' is declared but its value is never read.
 14   fontFamily,

Just removed the unused components and pushed back into the PR.

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

@timroes timroes removed the request for review from elastic/kibana-app Oct 9, 2019
@jbudz jbudz removed request for elastic/logs-metrics-ui Oct 9, 2019
@jbudz

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

Dropped a few labels, we'll let the apm team take this :). Thanks for the PR!

@sqren

This comment has been minimized.

Copy link
Member

commented Oct 15, 2019

retest

@sqren

This comment has been minimized.

Copy link
Member

commented Oct 15, 2019

@elasticmachine merge upstream

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2019

head repository does not exist

@sqren

This comment has been minimized.

Copy link
Member

commented Oct 15, 2019

@ffknob Can you please merge in the latest master? Then I'll merge this.

@elasticmachine

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2019

ffknob added a commit to ffknob/kibana that referenced this pull request Oct 15, 2019
@ffknob

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2019

Hi @sqren
Could you please verify this new PR I have created for this?
#48208

I'll explain:

  • Yesterday I had a problem and Ithought it would help to delete my Kibana fork
  • The thing is I didn't realized the links between open PRs and my repo would be forever lost
  • Then I was not able to rebase the branch that generated this PR to master anymore
  • So I created a new PR (in my new Kibana fork) with exactly the same change
  • In that PR I referenced this one here

So perhaps that one is the one to be merged and whis one should be closed...
The good thing is I think that one is already up to master.

@sqren

This comment has been minimized.

Copy link
Member

commented Oct 15, 2019

Closed in favor of #48208

@sqren sqren closed this Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.