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

style: admin view mobile #58

Merged
merged 5 commits into from
Dec 2, 2021
Merged

Conversation

akshay2000saxena
Copy link
Contributor

Notion ticket link

Ticket Name

Implementation description

image

image

image

image

Steps to test

What should reviewers focus on?

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@github-actions
Copy link

github-actions bot commented Dec 2, 2021

Visit the preview URL for this PR (updated for commit 9f9a9b7):

https://community-fridge-kw--pr58-akshay-feat-admin-vi-wvtxsrhc.web.app

(expires Thu, 09 Dec 2021 07:21:00 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@hanlinc27 hanlinc27 changed the title feat: admin view mobile stylt: admin view mobile Dec 2, 2021
@hanlinc27 hanlinc27 changed the title stylt: admin view mobile style: admin view mobile Dec 2, 2021
</Text>
)}
{isMobile && (
<Badge
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wondering if Badge should be a component...

Copy link
Member

Choose a reason for hiding this comment

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

it can, you can create a variant for it similar to what we have for the buttons in theme.tsx

Copy link
Member

@hanlinc27 hanlinc27 left a comment

Choose a reason for hiding this comment

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

lets goooooo
yippy-yay

Comment on lines +124 to +126
color={`${(colorMap as any)[schedule!.frequency]}.100`}
backgroundColor={`${(colorMap as any)[schedule!.frequency]
}.200`}
Copy link
Member

Choose a reason for hiding this comment

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

I think the color map needs to be edited to be "One time donation" for the key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh okay, lemme check with the designers to see what they finally decided on

@@ -46,8 +50,8 @@ const DefaultWeeklyEventItem = ({
borderColor="#D8DDE0"
alignItems="center"
centerContent
py="1.5rem"
px="2rem"
py={isMobile ? "1.25rem" : "1.5rem"}
Copy link
Member

Choose a reason for hiding this comment

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

since you're not doing any conditional rendering, I think all of the isMobile can just be

py={{base: "1.25rem", lg: "1.5rem"}} but this is fine!

@akshay2000saxena akshay2000saxena merged commit b3bb294 into main Dec 2, 2021
@hanlinc27 hanlinc27 deleted the akshay/feat/admin-view-mobile branch December 10, 2021 00:59
@hanlinc27 hanlinc27 restored the akshay/feat/admin-view-mobile branch February 13, 2022 23:27
@hanlinc27 hanlinc27 deleted the akshay/feat/admin-view-mobile branch March 17, 2022 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants