Added four preview images #535
Conversation
|
@jywarren Is everything ok in here? |
|
@publiclab/reviewers |
|
Hi @okonek . Your implementation looks pretty cool! Just as a suggestion, could we maybe display each image preview under its respective button and maybe allow them to click it?
|
|
Sure, that's a great idea. I'll do it. |
|
Ooh this is awesome. I love kevinzluo's suggestion too! Perhaps we could
even show the icon in white overlaid on the preview image?
…On Thu, Dec 6, 2018 at 2:38 PM Sidharth Bansal ***@***.***> wrote:
@SidharthBansal <https://github.com/SidharthBansal> requested your review
on: #535 <#535> Added
four preview images.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#535 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJxP6ErOL2rDrYvNspi_21ed58r24ks5u2XIegaJpZM4ZG5xY>
.
|
|
For example i know @JonathanXu1 developed some nice mockups for different projects and maybe could contribute some design ideas or even CSS code? |
|
I'd love to collaborate on this :) |
|
@JonathanXu1 Well, thank you very much, but I don't know if there is anything more to do in there. However if you have any idea of how it could be done better, let me know. I'd be very happy to collaborate with someone. |
|
@jywarren What do you think about this solution? Is it acceptable for you? |
|
How about something like this? (i've used stand-in icons and images but you get the idea)
@JonathanXu1 if you have suggestions that's awesome. @okonek in the future please just ask in a comment on the original issue to be sure someone's not already started -- even if they have, perhaps you can help them solve it! Thanks everyone! |
|
How about this: @JonathanXu1 can do some html/css design of the interface, but building on the code in this PR -- and then we merge the joint work. How does that sound? Also, just a shout out to @kevinzluo for the great design input as well! Good collaboration, folks! 🙌 |
|
Great idea. Is there a way to clone code from a pull request? |
|
I'm very sorry for taking your task, but I really didn't know. Maybe if someone has took the task like that it should be removed from GCI tasks list? |
|
@JonathanXu1 This could be helpful in creating a fork from a fork https://help.github.com/articles/creating-a-pull-request-from-a-fork/ Also remember that the code is on the branch |
|
@okonek is right about PR from a fork - the cool part about that will be that once @JonathanXu1's code is ready, we can merge it and it'll then appear in this PR and the tests will be re-run. Very meta! 🐢🐢🐢 @okonek no worries, it happens pretty often that folks don't see one another, as we're spread all over the world and the code is spread all over different parts of GitHub 😄 it's a lot to keep track of! I'm going to approve @okonek's portion of the task but make a new task instance for the UI part if @JonathanXu1 wants to take that up. It's your option! Thanks, all! |
|
This UI works looks really good!! Nice work @okonek 👏 |
|
How's this for the preview styling? |
|
It's great!!!! Could we actually constrain these to be square, using some
CSS? We might need to wrap them in a div and use `overflow:hidden`, make
the div square, and put the image inside it, and ensure the smallest
dimension of the image (height or width) is at least as large as the
container div?
…On Fri, Dec 7, 2018 at 3:09 PM Jonathan Xu ***@***.***> wrote:
How's this for the preview styling?
[image: image]
<https://user-images.githubusercontent.com/22998430/49670246-08569180-fa32-11e8-8eea-18f52c35975e.png>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#535 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJwGWX690VOlCqsy3y6hyOYY-HLLmks5u2srsgaJpZM4ZG5xY>
.
|
|
@jywarren Thank you! |
|
Hello! This is looking great! I think this task might have just been duplicated though. Look here: https://codein.withgoogle.com/tasks/6720201132343296/?sp-organization=6519545549291520&sp-is_beginner=False. |
|
This looks amazing!! Thanks a lot @JonathanXu1 😄 |
|
Could you also update the dist files here, just run |
|
Thanks! I'd like to do that but I don't have permissions to modify files |
|
I've made pull request jywarren#11 that applies my changes on top of @okonek 's. @jywarren can merge both our forks, or @okonek can apply my changes to his fork. |
|
It looked great and I merged it. Great work folks! |
|
So then we can close this one, right? Thanks!!!! |
|
Hi, could you rebase the remaining conflicts and then we'll merge this in? Thanks so much!!! |
|
I believe these changes were copied into #539 so there is no need to merge this one. |
|
I think you're correct. Thanks!!!! |
Fixes #247 [Add issue number here.]
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
rake test@publiclab/reviewersfor help, in a comment belowIf tests do fail, click on the red
Xto learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Thanks!