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

Refactored BidHistory layout, txnArrow was placed in incorrect row #72

Closed
wants to merge 3 commits into from

Conversation

dblodorn
Copy link
Collaborator

the transaction arrow (etherscan link) was positioned so that it appeared to related to the row beneath the one it actually was supposed to be placed in. Changed the markup to correct this.

Also added an optional selector prop to AddressView component so we don't have to wrap a span in a div or another span - but still have a selector to target for styling.

@dblodorn
Copy link
Collaborator Author

Layout change breaks BidHistory.test.tsx / @iainnash I can modify the test if you are ok with the change in markup.

Copy link
Contributor

@iainnash iainnash left a comment

Choose a reason for hiding this comment

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

Left a comment around exposing internal dom props. all else looks great!

const username = useZoraUsername(address);

const addressFirst = address.slice(0, showChars + PREFIX_ADDRESS.length);
const addressLast = address.slice(address.length - showChars);

if (username.username?.username) {
return <span>{`@${username.username.username}`}</span>;
return <span {...selector}>{`@${username.username.username}`}</span>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer doing className={} selector or just adding a wrapper class here. Allowing arbitrary props and classnames is a difficult thing to allow in framework code in my experience. You want to totally remove lower level presentational controls since now people can pass in onClick and such.
I’m okay with a dynamic classname but it may be better to just add a class to keep things consistent and allows us to easily search for how 3rd parties are styling this since we know the name of the class.

@dblodorn
Copy link
Collaborator Author

Moving this work over to a new branch:
https://github.com/ourzora/nft-components/tree/layout-cleanup

@dblodorn dblodorn closed this Sep 23, 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.

None yet

2 participants