Skip to content

New benchmark page #64

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
Merged
yjbanov merged 3 commits into from
Oct 6, 2016
Merged

New benchmark page #64

yjbanov merged 3 commits into from
Oct 6, 2016

Conversation

@yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Oct 6, 2016

Goals:

  • Remove dependency on Golem
  • Fit more benchmarks on one screen w/o sacrificing visibility too much
  • Open the possibility to set and track targets

Looks like this (showing randomly generated data):

screen shot 2016-10-06 at 12 54 09 pm

/cc @cbracken @sethladd

@sethladd
Copy link

sethladd commented Oct 6, 2016

What do we show when we regress or go below a target? Is it easy to identify the areas we need to focus on?

@yjbanov
Copy link
Contributor Author

yjbanov commented Oct 6, 2016

What do we show when we regress or go below a target? Is it easy to identify the areas we need to focus on?

Targets are not implemented yet. I'm open to suggestions, PRDs :)

cbracken
cbracken approved these changes Oct 6, 2016
View reviewed changes
Copy link
Member

@cbracken cbracken left a comment

Choose a reason for hiding this comment

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

Just a couple style nits. Looks good.

app/lib/components/benchmark_grid.dart Outdated
Copy link
Member

@cbracken cbracken Oct 6, 2016

Choose a reason for hiding this comment

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

Question: is Flutter style to push ctors above fields?

Copy link
Contributor Author

@yjbanov yjbanov Oct 6, 2016

Choose a reason for hiding this comment

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

Seen a lot of them above fields, e.g.:

https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/foundation/basic_types.dart#L64

So I'm guessing it's at least not against the style guide :)

app/lib/http.dart Outdated
Copy link
Member

@cbracken cbracken Oct 6, 2016

Choose a reason for hiding this comment

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

Flutter style: no curly brackets?

Copy link
Contributor Author

@yjbanov yjbanov Oct 6, 2016

Choose a reason for hiding this comment

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

Done

app/lib/model.dart Outdated
Copy link
Member

@cbracken cbracken Oct 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Last two lines: un-indent 1 col.

Copy link
Contributor Author

@yjbanov yjbanov Oct 6, 2016

Choose a reason for hiding this comment

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

Somehow I ended up with tabs here. Fixed.

app/lib/model.dart Outdated
Copy link
Member

@cbracken cbracken Oct 6, 2016

Choose a reason for hiding this comment

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

And here.

Copy link
Contributor Author

@yjbanov yjbanov Oct 6, 2016

Choose a reason for hiding this comment

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

Fixed.

@yjbanov yjbanov merged commit 3946dc8 into flutter:master Oct 6, 2016
@yusufm yusufm mentioned this pull request Nov 9, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cbracken cbracken cbracken approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yjbanov @sethladd @cbracken