Skip to content
This repository was archived by the owner on Mar 15, 2018. It is now read-only.

Conversation

@robhudson
Copy link
Contributor

No description provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh brother

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

@robhudson robhudson changed the title Updated to send platform and form_factor to API [Do not merge] Updated to send platform and form_factor to API Mar 24, 2014
@robhudson
Copy link
Contributor Author

@cvan Is this an r+? If not can you tell me what I need to do?

Also, this shouldn't be merged until the monster patch on zamboni is merged and after prod has done a full re-index. My goal is to merge this to target the push after the patch in zamboni lands and we have 1 week for prod to do a re-index. By sending the updated query string params we change the queries to use the new fields in ES.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A review on these widths for mobile and tablet, please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

address this please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not clear to me what you want me to do exactly. I see the widescreen capability. Do you want me to use 710px instead of 768?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah 710

Copy link
Contributor

Choose a reason for hiding this comment

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

do we use device_type anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, no more device_type.

@cvan
Copy link
Contributor

cvan commented Mar 26, 2014

there are several failing unit tests:

@robhudson
Copy link
Contributor Author

I ran the tests when I worked on this and they passed. I'll check it again. Thanks.

@cvan
Copy link
Contributor

cvan commented Mar 26, 2014

this is an r- until the tests are passing for me and the other comments are addressed - thanks 😏

@robhudson
Copy link
Contributor Author

All tests pass now after e0d45e1

@robhudson
Copy link
Contributor Author

Things have changed. Closing.

@robhudson robhudson closed this Apr 23, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants