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

Dpdb #103

Merged
merged 8 commits into from Jan 22, 2016
Merged

Dpdb #103

merged 8 commits into from Jan 22, 2016

Conversation

btco
Copy link

@btco btco commented Jan 15, 2016

Adds the DPDB file which contains DPI and bevel width information
for several popular devices. This is now used instead of the hard-coded
device parameters in device-info.js.

Bruno Oliveira added 2 commits January 14, 2016 17:58
Adds the DPDB file which contains DPI and bevel width information
for several popular devices. This is now used instead of the hard-coded
device parameters in device-info.js.
// Set the callback.
this.onDeviceParamsUpdated = onDeviceParamsUpdated;

console.log("Fetching DPDB...");
Copy link
Owner

Choose a reason for hiding this comment

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

nit: please use single quotes throughout.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@borismus
Copy link
Owner

Looks great overall, but I left some minor nits here and there.

@brianchirls
Copy link
Contributor

This is generally great - it's good to support as many devices as possible. Thanks for submitting. But I have a few issues in addition to the ones @borismus commented on...

  • The big JSON blob should be unminified in the repo. That way it will be readable and any future commits will make sense in diffs.
  • I see a lot of numerical values in there that are included as strings. Those quotes should be removed. It will make the file smaller, and there won't be any chance of non-numerical values accidentally slipping in later because they simply won't validate.
  • Similarly, dpi and res should be broken down into numbers rather than have the javascript parse them out.You could list them as two-element arrays. If you wanna be really compact, optionally allow a single number that is then interpreted as the value for both x and y.
  • I don't like that the file is loaded by XHR. If this project were using webpack, I'd recommend using a json loader with require, but as it is I'd rather see it included as code. That way whoever is using the code can decide on their own whether to concatenate or load asynchronously. And it can be minified better as code, eliminating unnecessary quotes. It will also allow for comments in the code in case there's something weird about one of the entries that needs to be explained.
  • (At the very least, a JSON file should have a .json extension, not .txt.)

@pindiespace
Copy link

How do these values compare to those on http://www.devicespecifications.com/
(it lists screen widths and heights independently of diagonals)

On Fri, Jan 15, 2016 at 7:46 PM, Brian Chirls notifications@github.com
wrote:

This is generally great - it's good to support as many devices as
possible. Thanks for submitting. But I have a few issues in addition to the
ones @borismus https://github.com/borismus commented on...

  • The big JSON blob should be unminified in the repo. That way it will
    be readable and any future commits will make sense in diffs.
  • I see a lot of numerical values in there that are included as
    strings. Those quotes should be removed. It will make the file smaller, and
    there won't be any chance of non-numerical values accidentally slipping in
    later because they simply won't validate.
  • Similarly, dpi and res should be broken down into numbers rather
    than have the javascript parse them out.You could list them as two-element
    arrays. If you wanna be really compact, optionally allow a single number
    that is then interpreted as the value for both x and y.
  • I don't like that the file is loaded by XHR. If this project were
    using webpack, I'd recommend using a json loader with require, but as
    it is I'd rather see it included as code. That way whoever is using the
    code can decide on their own whether to concatenate or load asynchronously.
    And it can be minified better as code, eliminating unnecessary quotes. It
    will also allow for comments in the code in case there's something weird
    about one of the entries that needs to be explained.
  • (At the very least, a JSON file should have a .json extension, not
    .txt.)


Reply to this email directly or view it on GitHub
#103 (comment)
.

Dr. Pete Markiewicz
Email: pindiespace@gmail.com
Sustainable virtual design blog:
http://sustainablevirtualdesign.wordpress.com
Sustainability template: http://greenboilerplate.com
Teaching site: *http://plyojump.com http://www.plyojump.com/
*Buy my book! *- Millennials and the Popular Culture
*Lifecourse:
http://www.lifecourse.com/store/catalog/lca/mpc.html

borismus added a commit that referenced this pull request Jan 22, 2016
@borismus borismus merged commit 6d6e7cd into borismus:master Jan 22, 2016
@brianchirls
Copy link
Contributor

This all looks interesting. I will dig in tomorrow and see if this has any effect on #102 and #105. It's definitely good that there's a default iOS device, which I imagine will at least prevent some breaking errors.

@btco, can you share a link to documentation on DPDB? I can't seem to find any info on who compiles it, how it works or how often it's updated? Thanks.

@btco
Copy link
Author

btco commented Jan 22, 2016

I'll add the documentation to the README soon!
On Jan 21, 2016 7:57 PM, "Brian Chirls" notifications@github.com wrote:

This all looks interesting. I will dig in tomorrow and see if this has any
effect on #102 #102
and #105 #105. It's
definitely good that there's a default iOS device, which I imagine will at
least prevent some breaking errors.

@btco https://github.com/btco, can you share a link to documentation on
DPDB? I can't seem to find any info on who compiles it, how it works or how
often it's updated? Thanks.


Reply to this email directly or view it on GitHub
#103 (comment)
.

@pindiespace
Copy link

Size matrix on this app page:
http://www.paintcodeapp.com/news/ultimate-guide-to-iphone-resolutions

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.

None yet

4 participants