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

issue #763 LCO List Component based on new Figma design #782

Merged
merged 12 commits into from
May 3, 2023
Merged

issue #763 LCO List Component based on new Figma design #782

merged 12 commits into from
May 3, 2023

Conversation

actuallyyun
Copy link
Collaborator

@actuallyyun actuallyyun commented Apr 17, 2023

Empty State

  • web

Screenshot 2023-04-19 at 11 47 35

  • mobile

Screenshot 2023-04-19 at 11 47 52

Happy Path

  • web
    Screenshot 2023-04-19 at 11 53 49

  • mobile
    Screenshot 2023-04-19 at 11 54 08

-Tooltip

Screenshot 2023-04-24 at 10 46 59

@vercel
Copy link

vercel bot commented Apr 17, 2023

@actuallyyun is attempting to deploy a commit to the openbeta-dev Team on Vercel.

A member of the Team first needs to authorize it.

@actuallyyun
Copy link
Collaborator Author

I had a close look at MobileDialog component, DialogTrigger element is a button, which does not work well with the new Figma design. Here the trigger is the whole LCO banner. Is it possible to customize DialogTrigger and make it behave like a container, or should I implement Dialog separately without using the existing components? What are your thoughts on this? @vnugent

@vnugent
Copy link
Contributor

vnugent commented Apr 18, 2023

I had a close look at MobileDialog component, DialogTrigger element is a button, which does not work well with the new Figma design. Here the trigger is the whole LCO banner. Is it possible to customize DialogTrigger and make it behave like a container, or should I implement Dialog separately without using the existing components? What are your thoughts on this? @vnugent

Let me have look at the Figma and then I can give you a better feedback. In general we're deprecating the headlessui library. Therefore please avoid using components that are based on that library.

Is it possible to customize DialogTrigger and make it behave like a container

If you use Radix UI trigger then Yes you can customize the trigger using your own component. Look into the asChild property.

Ps: also when ready please share some screen shots

@vercel
Copy link

vercel bot commented Apr 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
open-tacos ✅ Ready (Inspect) Visit Preview May 3, 2023 7:43am

@actuallyyun
Copy link
Collaborator Author

headlessui library. Therefore please avoid using components that are based on that library.

Super good to know! Should I try to stay with Radix UI then?
Will add screenshot, thanks for the reminder.

@vnugent
Copy link
Contributor

vnugent commented Apr 18, 2023

Regarding implementation for trigger it depends on whether we decide the LCO profile card to be a hover card (open on hover) or a modal popup (open on click).

Add area modal

Screenshot 2023-04-18 at 4 32 42 PM

https://github.com/OpenBeta/open-tacos/blob/develop/src/components/edit/Triggers.tsx#L95


I think for the scope of this PR, you can simply let users click on the LCO website link and we can figure out the LCO card in another PR.

LCO icon placeholder: the users hero icon will do for now

import {  UsersIcon } from '@heroicons/react/24/outline'

@actuallyyun
Copy link
Collaborator Author

Cool I will finish up the UI. How about the info icon tooltip? Should I leave it out for now as well? @vnugent

@vnugent
Copy link
Contributor

vnugent commented Apr 20, 2023

Can you fix linting errors? I can merge it and you can do a follow up PR for tool tips and other things.

@actuallyyun
Copy link
Collaborator Author

Can you fix linting errors? I can merge it and you can do a follow up PR for tool tips and other things.

fixed!

@actuallyyun actuallyyun temporarily deployed to Preview April 20, 2023 09:09 — with GitHub Actions Inactive
@actuallyyun
Copy link
Collaborator Author

Should I undo the lco test data before you merge it?

@vnugent
Copy link
Contributor

vnugent commented Apr 20, 2023

Should I undo the lco test data before you merge it?

yes please. We want the existing LCO info to continue to work.

@actuallyyun
Copy link
Collaborator Author

Removed tooltip and test data.

src/components/lco/PageBanner.tsx Outdated Show resolved Hide resolved
src/components/lco/PageBanner.tsx Outdated Show resolved Hide resolved
src/components/lco/PageBanner.tsx Show resolved Hide resolved
@vnugent
Copy link
Contributor

vnugent commented Apr 26, 2023

@actuallyyun let me know when it's ready to merge. You'll need to resolve some conflicts in cragSummary.tsx

@actuallyyun
Copy link
Collaborator Author

1

@vnugent ready to merge!

@vnugent
Copy link
Contributor

vnugent commented Apr 28, 2023

Did the build and linter pass locally in your env? (you can commit without --no-verify flag)

https://github.com/OpenBeta/open-tacos/actions/runs/4820193159/jobs/8584351722?pr=782#step:5:10

@actuallyyun actuallyyun changed the title [wip]issue #763 LCO List Component based on new Figma design issue #763 LCO List Component based on new Figma design Apr 28, 2023
@actuallyyun
Copy link
Collaborator Author

Sorry I must have reversed the linting errors I had fixed earlier when I resolved the merge conflicts. It should be fixed now.

@vnugent
Copy link
Contributor

vnugent commented Apr 28, 2023

@zichongkao I think recent changes to the LCO history object breaks the frontend build

https://github.com/OpenBeta/open-tacos/actions/runs/4829298519/jobs/8604171055?pr=782#step:8:43

@zichongkao
Copy link
Contributor

Thanks for pointing this out! I'll take a look and put in a fix by today!

@zichongkao
Copy link
Contributor

zichongkao commented Apr 28, 2023

This should fix it: #805 together with OpenBeta/openbeta-graphql#276. But we need the openbeta-graphql changes to be deployed so the open-taco change can be built before we merge it. Then I'll rebase this PR onto these changes!

@zichongkao zichongkao mentioned this pull request Apr 29, 2023
@vnugent
Copy link
Contributor

vnugent commented Apr 29, 2023

#805 merged. @zichongkao or @actuallyyun can you please rebase this PR then we're good to merge. Thank you!

@zichongkao
Copy link
Contributor

@actuallyyun This is actually a branch on your repo, so you'll have to rebase -- I can't do it. Sorry for the trouble!

@actuallyyun
Copy link
Collaborator Author

actuallyyun commented May 2, 2023

Rebased and pushed

@vnugent vnugent merged commit b76b33d into OpenBeta:develop May 3, 2023
actuallyyun added a commit that referenced this pull request May 7, 2023
* feat:move lco banner from left to right

* feat:if no lco found, show empty state

* use custom background color

* replace info icon svg with react component

* add previous hover card implementation in comments

* change LCO logo placeholder to UsersIcon

* style:use de-emphasized color for empty lco state text

* fix linting errors

* remove tooltip

* click on lco link opens a new tag

* add tooltip to show learn more

* fix linting errors
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

3 participants