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

Small changes #689

Open
wants to merge 7 commits into
base: staging
Choose a base branch
from
Open

Small changes #689

wants to merge 7 commits into from

Conversation

stevestriker
Copy link
Collaborator

Per request, added the underline to the proposal and removed the underline from the eth-address of the creator

@vercel
Copy link

vercel bot commented Nov 18, 2021

@stevestriker is attempting to deploy a commit to the 1hive Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Member

@fabriziovigevani fabriziovigevani left a comment

Choose a reason for hiding this comment

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

Thanks @stevestriker

Left some comments, let me know wyt

@@ -1,4 +1,5 @@
import React, { useEffect, useState } from 'react'
import { Link } from 'react-router-dom'
Copy link
Member

Choose a reason for hiding this comment

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

As discussed on Discord, let´s use the Link component from @1hive/1hive-ui.

Copy link
Member

Choose a reason for hiding this comment

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

You an see an example here

<EthIdenticon address={proposal.creator} radius={50} scale={1.8} />
)}
</div>
<Link to={`/profile?account=${proposal.creator}`}>
Copy link
Member

Choose a reason for hiding this comment

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

With the previous comment, this should be now

Suggested change
<Link to={`/profile?account=${proposal.creator}`}>
<Link href={`/profile?account=${proposal.creator}`} external={false}>

border-radius: 50%;
display: block;
object-fit: cover;
cursor: pointer;
Copy link
Member

Choose a reason for hiding this comment

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

Using the Link component suggested we don´t need to set this style

Suggested change
cursor: pointer;

radius={50}
scale={1.8}
css={`
cursor: pointer;
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above

<FixedFooter />
) : (
<Layout paddingBottom={70}>
<FixedFooter />
Copy link
Member

Choose a reason for hiding this comment

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

Why did we remove the compactMode condition? Note that we only want the Fixed Footer on smaller screen sizes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed cursor:pointer.
Fixed the href properties to direct correctly.
Still need to configure the compact mode footer tooldbar.

@0xGabi 0xGabi changed the base branch from gardens to staging December 22, 2021 23:35
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.

3 participants