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

Animated logo carousel #4459

Merged
merged 13 commits into from
Apr 12, 2023
Merged

Animated logo carousel #4459

merged 13 commits into from
Apr 12, 2023

Conversation

yathomasi
Copy link
Contributor

@yathomasi yathomasi commented Apr 7, 2023

Closes #4214

@yathomasi yathomasi self-assigned this Apr 7, 2023
@shcheklein shcheklein temporarily deployed to dvc-org-moving-company--r5kosr April 7, 2023 11:19 Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023

Link Check Report

There were no links to check!

@yathomasi yathomasi temporarily deployed to dvc-org-moving-company--r5kosr April 7, 2023 11:37 Inactive
@yathomasi yathomasi marked this pull request as ready for review April 7, 2023 11:39
@yathomasi yathomasi requested a review from a team as a code owner April 7, 2023 11:39
Copy link
Contributor

@omesser omesser left a comment

Choose a reason for hiding this comment

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

Looks great @yathomasi !! ❤️
Suggested some rephrasing for the sentence, PTAL

<div className="mb-5">
<h2 className={cn('text-2xl font-medium text-center', styles.title)}>
Empowering from startups to Fortune 500 Companies: Our Open Source
Tools at Work
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest this:

Empowering thousands of users and customers from startups to fortune 500 companies

notes:

  • avoiding open source mention - some use FOSS, some paid, and future rebranding might make this more misleading, so keeping it general
  • sentence case (not title case)
  • don't think we need ":" here (also avoided ",") - will look cleaner imo

Copy link
Contributor

Choose a reason for hiding this comment

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

@omesser

avoiding open source mention - some use FOSS, some paid, and future rebranding might make this more misleading, so keeping it general

Just for context: We have on purpose made the dvc.org website about open source (see hero).
All of these logos will not be able to be used for Studio. They can only be used for DVC website (or eventual page in rebranding) except for the ones that came from the Iterative site. So the open-source part is not an issue here.

That being said, I like your edit. Because open-source is mentioned in the hero, this simplifies the idea into one thought: EMPOWER, and lets people reflect on logos. I like it.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have on purpose made the dvc.org website about open source (see hero).

Yep, that makes total sense @jendefig - makes sense IMO to refer here to both users and customers, in the page-context of open source like you mentioned!

@jendefig
Copy link
Contributor

jendefig commented Apr 7, 2023

OMIGOD!!!! LOOOOOOOVE!!!! 🙌🏼

Couple things:
Can we move the sentence under the logos. or in some way tighten it up because on my screen I can't see the logos when I arrive:
image

I'm noticing that if you put the sentence under the logos, it will look awkward with the next section. (which looks a bit awkward now because white on white sections) We may need to do something about the background in the next section.

Possible easy fix that might be helpful in the meantime until we figure out the rest: Duplicate the bottom block to join the newsletter after the logos and then continue with the rest of the page. Might be nice to test if this makes an impact while we are figuring out next steps. It has little function at the bottom of the page. Most of our new newsletter emails are coming from the course.

And one more thing. Can we increase the speed a bit? I think the logos should move faster.

Also, Can you move UBS further up in the lineup - Let's put it after InLAB. Then I noticed that once they go through there is a flicker and they jump back after one cycle. Can you double the set of logos? At that point, no one will likely ever see the flicker. If they really want to sit through all the logos they will notice the repeat and move on.

Copy link
Contributor

@jendefig jendefig left a comment

Choose a reason for hiding this comment

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

Love it! See the other comment I put separately in my excitement! 👆🏽

@yathomasi yathomasi temporarily deployed to dvc-org-moving-company--r5kosr April 9, 2023 14:56 Inactive
@omesser omesser changed the title Moving companies logos Animated company logo carousel Apr 9, 2023
@omesser omesser changed the title Animated company logo carousel Animated logo carousel Apr 9, 2023
@yathomasi yathomasi temporarily deployed to dvc-org-moving-company--r5kosr April 10, 2023 07:42 Inactive
@yathomasi
Copy link
Contributor Author

Thanks @omesser and @jendefig for the suggestions.

tighten it up because on my screen I can't see the logos when I arrive

moved up to hero section so it should be visible now

it will look awkward with the next section

Again, now we moved to hero section the coloring looks good to me

join the newsletter after the logos and then continue with the rest of the page.

Sure, we can test that. I will push an update soon.

Can we increase the speed a bit?

I think it's more faster than previous and few other optimizations

Can you move UBS further up in the lineup - Let's put it after InLAB.

Done, right after InLAB. Let me know if we have more similar preferences.

Then I noticed that once they go through there is a flicker and they jump back after one cycle. Can you double the set of logos?

As mentioned, few optimizations are in place so that the flicker seems invisible or slight. We already had double set, now I multiplied it few more times and it should be negligible now.

Empowering thousands of users and customers from startups to fortune 500 companies

Updated title with

  1. fortune -> Fortune
  2. Also, added : to end similar to (https://iterative.ai/). Let me know if we have strong opinion on this.

@yathomasi yathomasi temporarily deployed to dvc-org-moving-company--r5kosr April 10, 2023 08:29 Inactive
@omesser
Copy link
Contributor

omesser commented Apr 10, 2023

fortune -> Fortune
Also, added : to end similar to (https://iterative.ai/). Let me know if we have strong opinion on this.

I personally think it looks a bit jarring and is slicker without the capital "F" and the ":". But I don't feel strongly about this that much 😄 , definitely not blocking
Will let you and @jendefig decide.
I think there's some restyled errors still - I'm approving to unblock - great job @yathomasi 🙌

@omesser omesser self-requested a review April 10, 2023 13:11
@yathomasi yathomasi temporarily deployed to dvc-org-moving-company--r5kosr April 11, 2023 04:23 Inactive
@yathomasi yathomasi added the p1-important Active priorities to deal within next sprints label Apr 11, 2023
@omesser
Copy link
Contributor

omesser commented Apr 11, 2023

@yathomasi - can you TAL at the restyled error please? is it a false negative?

@omesser omesser requested a review from jendefig April 11, 2023 13:52
@yathomasi
Copy link
Contributor Author

@yathomasi - can you TAL at the restyled error please? is it a false negative?

Kind of, it's not actionable on our side. I had an issue open on their side restyled-io/restyler#204 and as mentioned the solution is not applicable or worth the effort. So, I believe it's okay to skip that.

Copy link
Contributor

@jendefig jendefig left a comment

Choose a reason for hiding this comment

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

Love it!
Regarding Fortune 500 - It should be capitalized. It came from the magazine Fortune, so capitalized is correct.

The only thing is that there a way that the logos can fade out at the actual sides of my screen as opposed to the abbreviated one in line with the copy?
image

Is that because it will crash into the what's new button? Can we move the what's new button out of the way entirely? I see that if it is just up in the hero section it will crash into things on the left, but could it be on the right under the video?

@yathomasi
Copy link
Contributor Author

Thanks, @jendefig.

The width is kept the same as the hero section so that it doesn't look overflowing.

Actually, we can increase the width of the hero section up to how much the navbar is covering. But, for now, we can go ahead and merge this and do a little follow-up if needed.

Current Increased Width

@jendefig
Copy link
Contributor

so that it doesn't look overflowing.

We want the logos to look overflowing. Before the changes they went completely across my screen. See my previous picture.

@yathomasi
Copy link
Contributor Author

We want the logos to look overflowing. Before the changes they went completely across my screen. See my previous picture.

Screenshot 2023-04-12 at 09 12 16

This is how it would look and we wouldn't recommend it for a few good reasons:

  • we don't have content in the whole page overflowing outside the current container limit and we would want to be consistent
  • The main reason we don't want to hit the screen is it will not have great UX for large screens, wide screen monitors

Screenshot 2023-04-12 at 09 19 21

  • Also, I don't think we want to overhaul with lots of logos

@yathomasi
Copy link
Contributor Author

Merging this as we don't have any major blocker. We can definitely do follow-ups if anything comes up. cc: @jendefig

@yathomasi yathomasi merged commit 5e7fe5f into main Apr 12, 2023
@yathomasi yathomasi deleted the moving-company-logos branch April 12, 2023 05:27
@dberenbaum
Copy link
Contributor

Great work @yathomasi @jendefig! Looks nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1-important Active priorities to deal within next sprints
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Animated logo carousel with users & customer companies
5 participants