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

feat: wrap comments from github bot in details/summary #135

Merged
merged 3 commits into from
Feb 6, 2019

Conversation

nolanlawson
Copy link
Contributor

Details

This is step 1 in fixing #132. All it does is wrap the existing bot output in a details/summary tag, so that it can be clicked to be expanded.

Screenshot when collapsed:

screen shot 2019-01-30 at 3 13 52 pm

Screenshot when expanded:

screen shot 2019-01-30 at 3 13 45 pm

Of course, step 2 is to put useful summary info into the summary section, but this is at least a start. :)

Does this PR introduce a breaking change?

  • Yes
  • No

@salesforce-best-lwc-internal
Copy link

Benchmark results

Click for full results  

Base commit: e7ed35b | Target commit: ab3c960

simple-benchmark-prod

object_keys metric base(e7ed35b) target(ab3c960) trend
stringify_parse duration 27.00 (±0.10 ms) 27.50 (±0.20 ms) +0.5ms (1.9%) 👎
deep_merge duration 20.30 (±0.30 ms) 20.60 (±0.40 ms) +0.3ms (1.5%) 👎

simple-benchmark-compat

object_keys metric base(e7ed35b) target(ab3c960) trend
stringify_parse duration 27.50 (±0.20 ms) 26.90 (±0.20 ms) -0.6ms (2.2%) 👍
deep_merge duration 64.50 (±2.40 ms) 62.05 (±1.10 ms) -2.5ms (3.8%) 👌

lwc-examples-prod

simple-item metric base(e7ed35b) target(ab3c960) trend
create_and_render duration 25.90 (±0.40 ms) 26.40 (±0.50 ms) +0.5ms (1.9%) 👎

lwc-examples-compat

simple-item metric base(e7ed35b) target(ab3c960) trend
create_and_render duration 47.40 (±0.60 ms) 48.40 (±0.60 ms) +1.0ms (2.1%) 👎

@nolanlawson
Copy link
Contributor Author

Just realized that the thumbs up/down was wrong in the test fixture, so I fixed that.

@salesforce-best-lwc-internal
Copy link

Benchmark results

Click for full results  

Base commit: e7ed35b | Target commit: 9a17cd4

simple-benchmark-prod

object_keys metric base(e7ed35b) target(9a17cd4) trend
stringify_parse duration 27.00 (±0.10 ms) 27.90 (±0.20 ms) +0.9ms (3.3%) 👎
deep_merge duration 20.30 (±0.30 ms) 21.00 (±0.40 ms) +0.7ms (3.4%) 👎

simple-benchmark-compat

object_keys metric base(e7ed35b) target(9a17cd4) trend
stringify_parse duration 27.50 (±0.20 ms) 27.10 (±0.20 ms) -0.4ms (1.5%) 👍
deep_merge duration 64.50 (±2.40 ms) 64.50 (±2.30 ms) 0.0ms (0.0%) 👎

lwc-examples-prod

simple-item metric base(e7ed35b) target(9a17cd4) trend
create_and_render duration 25.90 (±0.40 ms) 25.90 (±0.30 ms) 0.0ms (0.0%) 👌

lwc-examples-compat

simple-item metric base(e7ed35b) target(9a17cd4) trend
create_and_render duration 47.40 (±0.60 ms) 47.70 (±0.60 ms) +0.3ms (0.6%) 👎

@diervo
Copy link
Contributor

diervo commented Feb 1, 2019

@nolanlawson I would love to have a holistic design of how we would like to display the results: what is important, what is not.

Copy link
Member

@pmdartus pmdartus left a comment

Choose a reason for hiding this comment

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

Couple of minor changes, except that LGTM.

@salesforce-best-lwc-internal
Copy link

Benchmark results

Click for full results  

Base commit: e7ed35b | Target commit: 2317adc

simple-benchmark-prod

object_keys metric base(e7ed35b) target(2317adc) trend
stringify_parse duration 27.00 (±0.10 ms) 28.20 (±0.30 ms) +1.2ms (4.4%) 👎
deep_merge duration 20.30 (±0.30 ms) 21.00 (±1.30 ms) +0.7ms (3.4%) 👎

simple-benchmark-compat

object_keys metric base(e7ed35b) target(2317adc) trend
stringify_parse duration 27.50 (±0.20 ms) 27.60 (±0.30 ms) +0.1ms (0.4%) 👎
deep_merge duration 64.50 (±2.40 ms) 64.90 (±2.50 ms) +0.4ms (0.6%) 👎

lwc-examples-prod

simple-item metric base(e7ed35b) target(2317adc) trend
create_and_render duration 25.90 (±0.40 ms) 27.30 (±0.70 ms) +1.4ms (5.4%) 👎

lwc-examples-compat

simple-item metric base(e7ed35b) target(2317adc) trend
create_and_render duration 47.40 (±0.60 ms) 51.30 (±1.50 ms) +3.9ms (8.2%) 👎

@diervo
Copy link
Contributor

diervo commented Feb 6, 2019

@nolanlawson you should be able to merge PRs now

@nolanlawson nolanlawson merged commit ec94f68 into salesforce:master Feb 6, 2019
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.

3 participants