-
Notifications
You must be signed in to change notification settings - Fork 1
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
[chore] Styling of Project Modal (w/o KDMs) #45
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks -- styling is really nice! Check my comments. Also make it so that the user can still interact with the google maps when the project modal is opened (you could do this in Ethan's latest PR so I think you might have made a change to a component / field)
components/ProjectModal/index.tsx
Outdated
/> | ||
<styles.ProjectOverview> | ||
<styles.Developer> | ||
<texts.BodyText1>Developer - {developer}</texts.BodyText1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to change the dash ( - ) to an arrow like in Josh's figma design? If not, let Josh know and see what he decides.
components/ProjectModal/index.tsx
Outdated
<styles.ProjectOverview> | ||
<styles.Developer> | ||
<texts.BodyText1>Developer - {developer}</texts.BodyText1> | ||
<styles.CloseButton onClick={closeModal}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make the pointer arrow become the hand when it hovers on a button (like how it normally looks on other websites)? You can use this for help https://stackoverflow.com/questions/3087975/how-to-change-the-cursor-into-a-hand-when-a-user-hovers-over-a-list-item . You just need to add cursor: pointer;
to the style.ts
Like this:
components/ProjectModal/index.tsx
Outdated
</Modal> | ||
<styles.ModalContent isOpen={openFirst}> | ||
<styles.ModalOverlay> | ||
<styles.SearchBar>Search</styles.SearchBar> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the search bar (this will directly be on the home page)
@@ -1,8 +1,9 @@ | |||
'use client'; | |||
|
|||
import React, { useEffect, useState } from 'react'; | |||
import Modal from 'react-modal'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our project modal component is built off of an already existing React Modal library that we are importing, so we shouldn't remove this.
@@ -46,39 +47,58 @@ export default function ProjectModal({ | |||
|
|||
return ( | |||
<div> | |||
<Modal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this modal component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just tested it out and I think that this is causing the issue. So instead of making a Modal styled component, revert to julee's previous code where he did:
<Modal isOpen={openFirst} style={{ overlay: styles.modalOverlayStyles, content: styles.modalContentStyles, }} >
You can keep everything else as a styled component though!
components/ProjectModal/index.tsx
Outdated
import { queryProjectbyId } from '../../api/supabase/queries/query'; | ||
import * as texts from '../../styles/texts'; | ||
import { Project } from '../../types/schema'; | ||
import * as styles from './styles'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, I think we should import all of the styled components individually as their names instead of as styles so that we don't have to start everything with "styles.---". It looks kinda hard to read with all of the "styles.---".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think in my previous project we also imported all styling individually instead of using styles. or texts. So I agree that we should change it to import individually. E.g. in texts.ts I imported all the fonts as: import { CoinbaseMono, CoinbaseSans, CoinbaseText } from './fonts';
instead of import * as fonts from ./fonts
I will also make this announcement on slack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks really good neha! you can squash and merge it.
What's new in this PR
Description
texts.ts
.Screenshots
Update:
How to review
pnpm dev
then press on one of the markers.Next steps
CC: @itsliterallymonique