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.

Sign up for GitHub

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

Jump to bottom

Create right info column with link #6

Merged
merged 5 commits into from
Dec 31, 2021
Merged

Create right info column with link #6

merged 5 commits into from
Dec 31, 2021

Conversation

GailWilson
Copy link
Contributor

@GailWilson GailWilson commented Dec 31, 2021 •
edited
Loading

  • Create InfoColumn
  • Create sub-components Link & InfoAccordion

Known issues:

  • Video is currently if a video URL is passed in it's just an image, we'll replace it with a proper video component when we have the content
  • Sizing/spacing is a bit funky but we'll do a sweep later

Screen Shot 2021-12-31 at 4 43 29 PM

@gentianmetaphor
Copy link

@GailWilson LG - if a video is added here, can it open full screen in a lightbox interaction?

@gentianmetaphor
Copy link

I believe this is outside the scope of this PR, but we should look at how all of these columns respond and how the components within them respond in the specific area's.

@GailWilson
Copy link
Contributor Author

@gentianmetaphor
RE: Video
This is just an image for now so we'll get to video functionality later on & will definitely add in the different interactions on what to do with it. Good call on the expanding to full screen. I won't build this component until we have content to add thoguh.

RE: Responsiveness
💯 We'll go through that, just making the base components to start & we'll do a sweep on responsiveness (making a linear ticket for that now)

@gentianmetaphor
Copy link

I think you're using 'button' for the headers rather than 'small button' text size

carlosdp
carlosdp reviewed Dec 31, 2021
View reviewed changes
src/InfoColumn.tsx Outdated
export interface InfoColumnProps {
icon?: string;
description?: string;
videoUrl?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: make these "Uri" instead of "Url", cause they could be ipfs/arweave URIs or something in the future

src/InfoColumn.tsx Outdated
description?: string;
videoUrl?: string;
linkText?: string;
linkUrl?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@GailWilson
Copy link
Contributor Author

Yep y'all are right! update ✅

@GailWilson GailWilson merged commit cd42a32 into master Dec 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlosdp carlosdp carlosdp left review comments

@gentianmetaphor gentianmetaphor gentianmetaphor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GailWilson @gentianmetaphor @carlosdp