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

feat: redesign creations page #114

Open
wants to merge 16 commits into
base: dev
Choose a base branch
from
Open

Conversation

0xShishigami
Copy link
Contributor

@0xShishigami 0xShishigami commented Oct 2, 2024

Changes done

  • Edit ProjectList
  • Add LogosCarousel
  • Filter projects by selected logo

Issue ticket number and link

Redesign projects section in creations page

closes PRIUI-264

Copy link

vercel bot commented Oct 2, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 4, 2024 5:20pm

Copy link

linear bot commented Oct 3, 2024

@0xShishigami 0xShishigami marked this pull request as ready for review October 3, 2024 16:51
@0xShishigami 0xShishigami requested a review from a team as a code owner October 3, 2024 16:51

export default function LogosCarousel({ companies, onChange }: LogosCarouselProps) {
const [loop, setLoop] = useState(carouselState.desktop.loop); // iterations of the logos
const [visibleItems, setVisibleItems] = useState(carouselState.desktop.visibleItems); // number of logos visible

Choose a reason for hiding this comment

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

I recommend avoiding commenting on every line and writing self-explanatory code.
loop state could be renamed to interations, or logoIteration.

visibleItems: it doesn’t need comments; it explains itself.
maxSize: the same, could be renamed to logosMaxSize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// setSize is used to determine the size of the logos
// if the index is the selected item, the size is the max size, Math.ceil(VISIBLE_ITEMS / 2)
// otherwise, if the index is bigger or smaller than the selected item, the size is the module of the difference between selected item and index
const setSize = (index: number, currIndex: number) => {

Choose a reason for hiding this comment

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

This function could be renamed to setLogoSizeByIndex and avoid extra comments, the code explains itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const handleChangeItem = (index: number) => {
if (index === selectedItem) return;

if (index === items.length - centerIndex) {

Choose a reason for hiding this comment

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

maybe use an or and avoid repeating code.

if(index === (items.length - centerIndex) || index === (centerIndex - 1)){
     resetIndex();
     return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

setSelectedItem(index);
if (onChange) onChange(index % companies.length); // get the index of the company

Choose a reason for hiding this comment

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

I do not recommend commenting code unless is strictly necessary.

if(onChange){
   const companyIndex = index % companies.length;
   onChange(companyIndex);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
}, []);

useEffect(() => {

Choose a reason for hiding this comment

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

This could be a useWindowSize hook, there are a lot of examples that you can follow and reuse in different carousels (and others), so you don't have to put this logic inside the carousel itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

@oxlumi oxlumi left a comment

Choose a reason for hiding this comment

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

Hey! I made some comments; here’s an overview:

  • Project Opening: What do you think about having only one project open at a time? If someone opens a new one, it should automatically close the last one. I think this could make navigation a bit smoother 😄

  • Text Size for "Made in Wonderland": Just a note that the "Made in Wonderland" text looks pretty big on some screens (like Mati's). Maybe we can shrink it a bit for better consistency?

  • Carousel Visibility: I chatted with Mati about showing the carousel as soon as you enter. If we drop the "We work with the best" section, it might give a cleaner look and allow people to dive into the projects right away

  • Projects JSON: I made some comments on specific details in this as well :)

  • Regarding the project list component, can we remove the ":" after the questions? Here’s an image for reference: image

"web": "https://www.connext.network/"
"name": "Crosschain Governance",
"company": {
"name": "Connext",
Copy link

Choose a reason for hiding this comment

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

  • We should change "Connext" for "Everclear" (in all the projects)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"content": {
"challenge": {
"title": "What was the challenge?",
"description": "MakerDAO is one of the largest and oldest governance in the Crypto DeFi ecosystem and as such has on a daily basis a large number of votes that need to be executed on-chain. These votes which can be payments, parameter changes in the voult, or a contract update are known as spells."
Copy link

Choose a reason for hiding this comment

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

Can we update the description here?

"description": "Sky (formerly MakerDAO) is one of the largest and oldest governance systems in DeFi. Every day, it oversees a significant number of on-chain votes that need to be executed. These votes can include payments, parameter adjustments within vaults, or contract updates, collectively referred to as spells."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{
"name": "Spells",
"company": {
"name": "MakerDAO",
Copy link

Choose a reason for hiding this comment

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

"name":"Sky",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"github": "https://github.com/defi-wonderland/xERC20"
"name": "Everclear",
"company": {
"name": "Connext",
Copy link

Choose a reason for hiding this comment

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

Same here "name":"Everclear",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"twitter": "https://twitter.com/amphoraprotocol"
"name": "Optimistic Merkle Root Aggregator",
"company": {
"name": "Connext",
Copy link

Choose a reason for hiding this comment

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

Same here, "name":"Everclear",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -32,7 +49,7 @@ export default function Creations() {
<HeroDivider>
<CreationsTitleContainer>
<SquigglyTitle
text='WHAT’S COOKING?'
text='MADE IN WONDERLAND'
sizes={{
Copy link

Choose a reason for hiding this comment

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

Just a quick note: the current version of the creations hero looks like this:
image

Mati and I were reviewing it, and on his computer, the "Made in Wonderland" text appears huge:
image

On my end, it looks more like this:
image

Could we consider reducing the text size for different screen resolutions? Not sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -49,15 +66,26 @@ export default function Creations() {
</HeroDivider>

<TitleContainer>
<ProjectTitle title='Partner projects' />
<ProjectTitle>
Copy link

Choose a reason for hiding this comment

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

Current version:
image

After discussing with Mati, we thought it might be better to display the small carousel as soon as you enter. So, we suggest removing the "We work with the best" section, resulting in something like this:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -41,48 +69,70 @@ export default function ProjectsList({ projects = [] }: ProjectListProps) {
};

return (
<styles.List>
<List>
{projects.map((project) => (
<ProjectContainer key={project.name}>
<ProjectHeader onClick={(e: React.MouseEvent<Element, MouseEvent>) => handleClick(project, e.currentTarget)}>
Copy link

Choose a reason for hiding this comment

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

Note: Consider implementing a single open state for projects. If one project is opened, the others should automatically close : )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@0xArdy 0xArdy left a comment

Choose a reason for hiding this comment

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

Some UI/UX comments:

  • the carousel in creations page should have cursor pointer
  • maybe we can add a hover effect for the project icons in the carousel
  • the user cant select the text in the project description, also it shouldn't have cursor pointer in that section
  • i think we should remove the : in the project description titles
    From: What was the challenge?:
    To: What was the challenge?

import { styled } from 'styled-components';
import { MOBILE_MAX_WIDTH, TABLET_MAX_WIDTH } from '~/components';
import { Carousel } from 'react-responsive-carousel';
import { useCallback, useEffect, useMemo, useState } from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

react imports should be on line 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

import { PageContent } from '~/containers/PageContent';
import LogosCarousel from './LogosCarousel';
import { useMemo, useState } from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

idem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@0xShishigami
Copy link
Contributor Author

Some UI/UX comments:

  • the carousel in creations page should have cursor pointer
  • maybe we can add a hover effect for the project icons in the carousel
  • the user cant select the text in the project description, also it shouldn't have cursor pointer in that section
  • i think we should remove the : in the project description titles
    From: What was the challenge?:
    To: What was the challenge?

@0xArdy
77d3055

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.

4 participants