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

Auction Banner #1023

Merged
merged 11 commits into from Jan 14, 2016
Merged

Auction Banner #1023

merged 11 commits into from Jan 14, 2016

Conversation

ashfurrow
Copy link
Contributor

Profile information is unavailable (see: https://github.com/artsy/ohm/issues/275 ) so I've hardcoded some values for now in the view model.

Still to be done for #974 is the countdown timer, alluded to in some TODO comments.

EDIT: Since this PR has expanded a bit since I opened it, I can now add the following:

Fixes #974.

prefix operator ^ { }
prefix func ^<T>(closure: T -> Void)(instance: T) {
return apply(closure)(instance: instance)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is really cool, but I'm a terrible judge of what is cool (see: high school). I'm cool with either a) a different operator, or b) no operator (just a function).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think apply is much better. I’m not a fan of custom operators outside of a DSL or strictly related to the domain they’re used in. Words are handy, as they describe what they mean ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Having said that, I do like the operator and could see this pattern being used a lot and thus having the reader learn about 1 operator is a low enough burden.

So let’s do it like this. Either you guarantee to use this operator all over the place over the next few weeks or remove it now and add it back later once apply is used all over the place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I'll take it out for now and we'll see how widely apply is used 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@alloy
Copy link
Contributor

alloy commented Jan 13, 2016

I’d like to see some screenshots/videos please.

@ashfurrow
Copy link
Contributor Author

Good point! I'll add some shortly.

@ashfurrow
Copy link
Contributor Author

OK, so the banner looks like this on iPhone:

screen shot 2016-01-13 at 10 40 49 am

And this on iPad:

screen shot 2016-01-13 at 10 41 21 am

Things to note:

I'll remove the ^ operator.

@ashfurrow
Copy link
Contributor Author

Oh, and the images are pngs but without a transparent background 🙈 We'll need to talk to the liaisons about that.

@orta
Copy link
Contributor

orta commented Jan 13, 2016

assigning to alloy, cause I can

@ashfurrow
Copy link
Contributor Author

Delegation awwww yissss

@ashfurrow
Copy link
Contributor Author

Cool, had some more time on this. Turns out there was already a countdown view 🎉 Love it when I don't have to do the work.

The countdown view's design itself is going to change across the board (Katarina is opening an issue), so ignore how it doesn't match the mocks for now.

iPhone:

screen shot 2016-01-13 at 4 45 20 pm

iPad:

screen shot 2016-01-13 at 4 46 05 pm

backgroundImageView.alignToView(self)
darkeningView.alignToView(self)

logoImageView.constrainWidth(nil, height: "70")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intent here to not add a width constraint? If so, please just use constrainHeight().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@alloy
Copy link
Contributor

alloy commented Jan 14, 2016

Small comments, otherwise good to merge after addressing those 👍

ashfurrow added a commit that referenced this pull request Jan 14, 2016
@ashfurrow ashfurrow merged commit 1d659ca into master Jan 14, 2016
@ashfurrow ashfurrow deleted the auction-banner branch January 14, 2016 16:14
@ashfurrow ashfurrow mentioned this pull request Jan 14, 2016
8 tasks
@ArtsyOpenSource
Copy link
Contributor

  1 Warning
⚠️ Needs testing on a Phone if change is non-trivial

Generated by 🚫 danger

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