pythonindia / inpycon2018

PyCon India 2018 site
https://in.pycon.org/2018/
Other
110 stars 61 forks source link

added popups for sponsors #187

Closed akshat0047 closed 6 years ago

akshat0047 commented 6 years ago

Issue #77 @pythonindia/pycon-2018-team

Added Popups -data is being parsed from json (schedule.json) -dom is controlled under function createpopupanim (schedule.js) -style.css updated -data is not populated in json for popups

Demo: https://akshat0047.github.io/inpycon2018/ Do review for changes in design.

akshat0047 commented 6 years ago

There are few styling improvements, will do it today to be reviewed.

ananyo2012 commented 6 years ago

So the current design is a vertical rectangle so we need to scroll a bit to see the full content. I think a horizontal design would be better as we don't have to scroll even if the content is bit long. Here is a sample mockup

mock

We can also add some keywords at the end of the description in the same badge format as the location. Keywords can be comma or space separated value in the data.

ananyo2012 commented 6 years ago

@akshat0047 Please make changes according to my comment above.

akshat0047 commented 6 years ago

Ok I'll make them asap.

ananyo2012 commented 6 years ago

Thanks for the prompt reply :)

akshat0047 commented 6 years ago

Also the design you suggested will not be compatible for small screens I think there it should be the way it is now?

ananyo2012 commented 6 years ago

You can use bootstratp grids within the popup. So for smaller screen widths it can be made vertical and the grids can stack vertically.

akshat0047 commented 6 years ago

Ok sure

ananyo2012 commented 6 years ago

@akshat0047 Any update on this ?

akshat0047 commented 6 years ago

No, for sure it'll be update the way suggested till tomorrow evening

realslimshanky commented 6 years ago

@akshat0047 make sure you also rebase your branch to resolve conflicts.

ananyo2012 commented 6 years ago

Hi @akshat0047 , Thanks a lot for your contribution. I had a talk with our Sessions coordinator and he said that it will be hard to get all the images for speaker at this point of time and we are already late announcing the finalized schedule. So its better to drop this feature for now.

That said I don't want your hard work to go in vain. We still need a description section for sponsors as per #78 . It will pretty much be the same just we need the same to be reflected in sponsors.json and sponsors.js. And the content will be just the logo, description and View More button in a popup. Keep the logo and description one after another unlike the horizontal look as the talks since the logo is of varied dimensions. Also make the popup of larger dimension since the content will be more.

Please update the PR as per the comment above. Thanks a lot for your hard work this far !

ananyo2012 commented 6 years ago

@akshat0047 Any update on this ?

akshat0047 commented 6 years ago

Made the changes, Please review @ananyo2012

realslimshanky commented 6 years ago

@akshat0047 I think 1) having the image on the popup is redundant and doesn't look good also. 2) It should be better to replace Details with Website image

ananyo2012 commented 6 years ago

Yeah I agree to @realslimshanky 's comment. Looking at the popup the logo isn't needed. Some more comments.

  1. please remove the badge and make the Company name bigger.
  2. Can we have a small close button at the top right corner ? Currently we have to click somewhere outside the logo to dismiss it which is a bit hard to find as the user is likely to click over the logo first

Please fix the above 2 comments as well.

ananyo2012 commented 6 years ago

Btw I like the design for the sponsor heading :+1:

ananyo2012 commented 6 years ago

@akshat0047 Any update on this ?

realslimshanky commented 6 years ago

@akshat0047 please also resolve conflicts.

ananyo2012 commented 6 years ago

@akshat0047 The popup is breaking for extra small screen-width, can you update it with proper media queries for extra small screen width ? I updated your branch with the company descriptions and rebased the changes to solve the merge conflicts that were there recently. Do a git reset --hard HEAD~1 and then do a git pull origin master to get the changes.

akshat0047 commented 6 years ago

Updated @ananyo2012

ananyo2012 commented 6 years ago

LGTM. This looks good 👍 Merging this. Thanks for your contribution!

akshat0047 commented 6 years ago

Welcome, this is my first contribution to a major repository, thank you to guide me for that long.

vipulgupta2048 commented 6 years ago

Good stuff, keep at it @akshat0047

ananyo2012 commented 6 years ago

That's great :) . We are happy to welcome more first timers to pythonindia projects!

On Tue 2 Oct, 2018, 3:18 AM Vipul Gupta, notifications@github.com wrote:

Good stuff, keep at it @akshat0047 https://github.com/akshat0047

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/pythonindia/inpycon2018/pull/187#issuecomment-426074898, or mute the thread https://github.com/notifications/unsubscribe-auth/AKACR-aP6CAaYfeJoNv17IVus6IiXlIqks5ugo2ugaJpZM4WoGZE .