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

testimonial: added carousel on landing page #80

Merged
merged 3 commits into from
Dec 8, 2017

Conversation

thelostone-mc
Copy link
Member

Changes:

  • created a fadein-fadeout carousel for testimonial on the landing page
  • populated with dummy content
  • responsive design
  • used font-awesome icons and shapes instead of the images

Refs: #63

@tra38 I rewrote the code base using yours as a starting point. Any thoughts on this approach ?
@owocki Also , the design laid out by @taurean was pretty sweet , but I decided to use font-awesome icons and basic shapes to recreate the design.

Could either of you spare a few min and make sure I've gotten everything right ?

Reason :

  • images are heavier
  • resizing divs and svg icons is a lot easier that images and they don't loose clarity

files added:

  • carousel.css
  • carousel.js
  • testimonials.js

files changed:

  • index.html
  • footer_scripts.html
  • head.html

@owocki
Copy link
Contributor

owocki commented Dec 5, 2017

#69
#80
#79

@tra38 @ethikz @thelostone-mc -- im thrilled that everyones excited about this and that we've got working testimonial modules, but i wish we had all coordinated better. it makes me sad we can't use all three 3 PRs. but i pulled down all 3 codebases and all 3 work, so that's pretty cool!

ill own not having a better claim tool. that's on gitcoin.

so... where do we go from here? whats best for the repo?

@taurean
Copy link

taurean commented Dec 5, 2017

looks like #80 is the closest to the mockup at the moment. the prev/next controls still need to be centered properly within the circle. The icons surrounded the user-photo is lacking that layered effect with the illustrative elements but that might be because that place-holder image is transparent? In either case, there should probably be a background color set to the image to mitigate that issue. The cited name should also be in caps. Thanks everyone!

@tra38
Copy link
Contributor

tra38 commented Dec 5, 2017

so... where do we go from here? whats best for the repo?

Since open source is built on collaboration, it would seem the best way of handling this scenario is to merge all the best ideas from the different PRs into a single PR to be merged into master. All people involved will share in the bounty, based on how much contribution each person did to the overall task.

I'm perfectly fine with @thelostone-mc rewriting my codebase. There's some features in the old codebase that I want to port over to the new one (such as allowing programmers to change slide timings), mostly by adding commits to this PR. Would that be okay with you, @thelostone-mc?

@thelostone-mc
Copy link
Member Author

thelostone-mc commented Dec 5, 2017

@taurean

  • Ah I didn't notice the mis-alignment on my machine. That looks ugly, will fix that
  • Yeah the layered effect isn't visible cause the avatar has a transparent background.I tested it out with a photo with a solid background, no issues there. We could set an overlay color, but I'm not sure if css has the ability to detect if the background is transparent and adding a default overlay on every photo might be a bit of an overkill. Any thoughts on this @owocki , @tra38 ?
  • will change the cited name to be in caps

@tra38 By all means -> port it over via a commit :D (For now, one has to edit this line here to change the timings )

@taurean
Copy link

taurean commented Dec 5, 2017

awesome @thelostone-mc. re: background image, I don't think you need to detect transparency first. setting a background color of #FAFAFA or something to img or the parent containing img and making sure it doesn't peak out underneath opaque images should work just fine. I can whip a codepen example up if thats helpful to illustrate what I mean.

@thelostone-mc
Copy link
Member Author

@taurean

  1. Setting the background to #FAFAFA

screen shot 2017-12-05 at 1 39 17 pm

  1. Setting the background to #FFF looks neater but then the layering effect gets compromised to an extent (Seems like the best option )

screen shot 2017-12-05 at 1 40 59 pm

  1. Setting the background of the parent to #FAFAFA looks perfect but it messes up when you have a photo with a non-transparent bg. (Ignore the positions of the circles for now )

Good:
screen shot 2017-12-05 at 1 53 50 pm

Bad:
screen shot 2017-12-05 at 1 54 20 pm

PS: Is the font-size for the cited name alright or does it need to be increased ?

@owocki
Copy link
Contributor

owocki commented Dec 5, 2017

thanks for working together everyone.. im thrilled that you're working together in the spirit of open source :)

let me figure out what to do about the bounty payout here while you all iron out the PR.

@owocki owocki mentioned this pull request Dec 6, 2017
@owocki
Copy link
Contributor

owocki commented Dec 6, 2017

@ethikz what do you think of this approach?

@tra38
Copy link
Contributor

tra38 commented Dec 6, 2017

@thelostone-mc, here's my commit with my suggested changes to your code. It's in a separate branch since I don't know how to push code onto your branch. Let me know how you like it.

@taurean
Copy link

taurean commented Dec 6, 2017

@thelostone-mc here is an example codepen of what I'm talking about, there shouldn't be any additional padding causing the issue in the screenshot I don't think. re: font-size I can't inspect that image to check but the computed size that's rendered for the block quote is 20px and the cited name is 16px. Both the block quote and cited name should be a medium weight so probably like font-weight: 600 if I had to guess?

@taurean
Copy link

taurean commented Dec 6, 2017

@owocki do you happen to know if the testimonial images will be photos or illustrations? Maybe we're thinking too much about setting fallbacks if we know the exact images we're building for.

@thelostone-mc
Copy link
Member Author

@tra38 Gimme a day! Let me fix the css up a bit and squash the commit, then just send out a PR to my branch , i'll merge it and it would be reflected here ( sound good ? )

@taurean so basically the first photo I'd posted ^_^ will do

@owocki
Copy link
Contributor

owocki commented Dec 6, 2017

@taurean i'm still gathering the images. do you think gitcoin style illustrations (like my avatar) would look good? if so, we could probably get them hooked up

@thelostone-mc
Copy link
Member Author

@tra38 I''ve made the changes (feel free to double check ^_^ )
Could you send out the PR ? I'll merge it and we'll get this PR set in motion :D

@tra38
Copy link
Contributor

tra38 commented Dec 6, 2017

@thelostone-mc: PR sent! 👍

- created a fadein-fadeout carousel for testimonial on the landing page
- populated with dummy content
- responsive design
- used font-awesome icons and shapes instead of the images

Refs: gitcoinco#63
… when slides change

Allowing users to control slideDuration via views.py makes it easy to change the duration
of each slide without having to affect the actual JS logic.

Resetting the timer whenever the slide changes fixes a bug that can occur
whenever the slide auto-advances while the user interacts with the carousel.
@thelostone-mc
Copy link
Member Author

@owocki I guess we are done from our end ^_^

@owocki
Copy link
Contributor

owocki commented Dec 6, 2017

sawwwweetttt.. rdy for me to test then? thelostone-mc:testimonial is the branch i want to test?

@thelostone-mc
Copy link
Member Author

@owocki That's the one \m/

@owocki
Copy link
Contributor

owocki commented Dec 7, 2017

pulled it down to QA.

  1. looks very nice. good work yall!
  2. can the cursor turn to a opinter when you hover over the left/right arrows?
  3. responsive works very well on chrome and on safari!

woo hoo!

}

.carousel .avatar .circle-small {
position: relative;
Copy link
Contributor

Choose a reason for hiding this comment

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

inconsistent tab / spacing on this file. can we clean that up real quick?

Copy link
Member Author

Choose a reason for hiding this comment

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

Haha getting that done !! Thanks for pointing that out ^_^

Copy link
Contributor

Choose a reason for hiding this comment

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

still looks inconsistent to me

@@ -26,7 +26,14 @@


def index(request):
slides = [
Copy link
Contributor

Choose a reason for hiding this comment

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

i love that this is abstracted out of the template :)

Copy link
Member Author

Choose a reason for hiding this comment

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

credits go to @tra38 :D

context = {
'slides': slides,
'slideDuration': 6000,
Copy link
Contributor

Choose a reason for hiding this comment

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

ms

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup! we could rename it to slideDurationInMs or leave a comment -> take your pick

Copy link
Contributor

Choose a reason for hiding this comment

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

up to you

@owocki
Copy link
Contributor

owocki commented Dec 7, 2017

just left a few comments. then i think this is rdy to merge!

@taurean
Copy link

taurean commented Dec 7, 2017

chiming in really quick, looks like the controls for navigating to other testimonials are divs, ideally these should be buttons for accessibility reasons.

@thelostone-mc
Copy link
Member Author

@owocki

  • re-indented the css file
  • not sure if i should rename the timer variable to slideDurationInMs

can the cursor turn to a opinter when you hover over the left/right arrows?

Yup!

responsive works very well on chrome and on safari!

and firefox \m/

@taurean I meant to do that. Slipped my mind! Fixed ^_^

@ethikz
Copy link
Contributor

ethikz commented Dec 7, 2017

@owocki Sorry I didn't get any notification 😞

What approach are you referring to?

@owocki
Copy link
Contributor

owocki commented Dec 7, 2017

What approach are you referring to?
@ethikz
the one on this PR. there was a mix up and multiple people workedo n this. see => #80 (comment)

- carousel control are now buttons
- mouse turns to pointer on hover over control
- re-indented code
@thelostone-mc
Copy link
Member Author

@owocki changes done ^_^

@ethikz
Copy link
Contributor

ethikz commented Dec 7, 2017

@owocki Oh...yea I saw. Whichever one is better I'd use. I threw it together quickly so maybe @thelostone-mc was able to provide a better solution

@owocki
Copy link
Contributor

owocki commented Dec 7, 2017

i have a few testimonials sourced and i am just circling back with everyone to make sure they're double extra secret sure they're okay with being quoted publicly. i hope to have that content added to the PR by EOW

@owocki
Copy link
Contributor

owocki commented Dec 8, 2017

ok ive got the testimonials sourced. testing for merge now

@owocki owocki merged commit 7f0a990 into gitcoinco:master Dec 8, 2017
@owocki
Copy link
Contributor

owocki commented Dec 8, 2017

https://gitcoin.co <= hows it look

heres the commit for adding the real testimonials 729710f

@gitcoinbot
Copy link
Member

⚡️ A tip worth 0.075 ETH has been granted to @tra38 for this issue from Kevin Owocki. ⚡️

Nice work @tra38, check your email for further instructions. | Send a Tip

@gitcoinbot
Copy link
Member

⚡️ A tip worth 0.075 ETH has been granted to @ethikz for this issue from Kevin Owocki. ⚡️

Nice work @ethikz, check your email for further instructions. | Send a Tip

@owocki
Copy link
Contributor

owocki commented Dec 8, 2017

thanks yall :)

@ethikz
Copy link
Contributor

ethikz commented Dec 8, 2017

@owocki Looks good! Minor issue in the nav, arrows aren't exactly centered but other than that works and looks great

@gitcoinbot
Copy link
Member

⚡️ A tip worth 0.075 ETH has been granted to @thelostone-mc for this issue from Kevin Owocki. ⚡️

Nice work @thelostone-mc, check your email for further instructions. | Send a Tip

@thelostone-mc thelostone-mc deleted the testimonial branch December 11, 2017 21:01
ethikz pushed a commit to ethikz/web that referenced this pull request Jan 24, 2018
testimonial: added carousel on landing page
owocki pushed a commit that referenced this pull request Apr 30, 2021
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.

6 participants