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

Add basic browser and platform into to changeset tags #2559

Merged
merged 2 commits into from
Mar 21, 2015
Merged

Conversation

bhousel
Copy link
Member

@bhousel bhousel commented Mar 20, 2015

This adds some simple info to the changeset tags about browser and platform used.
Reporting this info is useful for some things (see #2449).

  • Browser version is detected as major.minor only (no patch included).
  • Platform is very simple (Windows/Macintosh/Linux) with no version.
  • These fields are namespaced with 'iD:' to not conflict anything

Looks like this in practice:

screenshot 2015-03-20 14 27 37

cc @pnorman

@pnorman
Copy link
Contributor

pnorman commented Mar 20, 2015

If the tags aren't iD specific and are clear in their meaning, I think avoiding namespaces are best.

P2 can run on multiple sites, and all editors have a locale. Browser is less relevant for P2, but any future HTML or JavaScript editors will probably find it relevant.

@bhousel
Copy link
Member Author

bhousel commented Mar 20, 2015

hmm.. platform is already a key.. Maybe change to operating_system and remove the namespaces?

@pnorman
Copy link
Contributor

pnorman commented Mar 20, 2015

hmm.. platform is already a key

Used in changeset tags?

bhousel added a commit that referenced this pull request Mar 21, 2015
Add basic browser and platform into to changeset tags
@bhousel bhousel merged commit 1d2b613 into master Mar 21, 2015
@bhousel bhousel deleted the changeset-info branch March 21, 2015 02:30
@ImreSamu
Copy link

ImreSamu commented May 4, 2015

detected.platform = 'Linux';
}
else {
detected.os = 'win';
Copy link

Choose a reason for hiding this comment

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

This does seem odd. If we cant detect anything, we set the detected os to windows? Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

detected.os is used for only one thing in iD, and that is to setup the keyboard shortcuts in cmd.js.

I suppose win was just picked as a conservative default for the keyboard shortcut setup. In reality, if the code runs here, the user is probably on a device without a keyboard - like a game console or an Android phone.

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.

5 participants