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

New UI #1890

Closed
wants to merge 30 commits into from
Closed

New UI #1890

wants to merge 30 commits into from

Conversation

ororsatti
Copy link
Collaborator

@ororsatti ororsatti commented Sep 19, 2024

This PR implement all the new UI components, and the new background.
We made sure to not touch the existing app.

Showcase:

Inputs:

Screen.Recording.2024-09-22.at.21.47.52.mov

Buttons:

Screenshot 2024-09-22 at 21 48 32

List:

Screenshot 2024-09-22 at 21 48 50

Accordion:

Screen.Recording.2024-09-22.at.21.49.08.mov

POD Card:

Screenshot 2024-09-22 at 21 49 26 Screenshot 2024-09-22 at 21 49 34

Ticket:

Screenshot 2024-09-22 at 21 50 08

Modal:

Screen.Recording.2024-09-22.at.21.52.22.mov

@rrrliu rrrliu requested a review from ichub September 19, 2024 21:33
Copy link
Contributor

@ichub ichub left a comment

Choose a reason for hiding this comment

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

lgtm after suggestions

import { InputHTMLAttributes, Ref } from "react";
import styled from "styled-components";

export const BigInput = styled.input`
Copy link
Contributor

Choose a reason for hiding this comment

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

for reimplementations of existing components, i would prefer their names to not overlap with the existing components, so as to not hinder development on existing UIs. Something like BigInput2 would work for me

inputProps: EmailCodeInputProps
): JSX.Element => {
return (
<BigInput
Copy link
Contributor

Choose a reason for hiding this comment

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

curious whether this opens up the digit input UI on mobile

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMG_3374

import React from "react";
import styled from "styled-components";

const TypographyText = styled.span`
Copy link
Contributor

Choose a reason for hiding this comment

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

slightly unclear what this is for - typography is very vague as a name. are you just trying out the font here? if so that's fine but leave a comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not only, but also define a set of sizes and styles for our texts,

apps/passport-client/pages/index.tsx Show resolved Hide resolved
auto-merge was automatically disabled September 21, 2024 12:36

Head branch was pushed to by a user without write access

Copy link
Contributor

Choose a reason for hiding this comment

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

this will probably need to be copied into the CardBody component of both EdDSATicketPCD and PODTicketPCD. I suggest creating a boolean prop for both of those components that lets the caller of those components to switch between the old and new implementations of the UIs.

  • packages/ui/eddsa-ticket-pcd-ui/src/CardBody.tsx
  • packages/ui/pod-ticket-pcd-ui/src/CardBody.tsx

we can do that in a follow-up PR - it's not blocking. but I would definitely like to get it done before the end of phase 3.

that might mean some of the UI components that you've implemented in passport-client might need to be moved to passport-ui

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it, we will follow this up

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the reason for this though? So we will reuse the component logic?

Copy link
Contributor

Choose a reason for hiding this comment

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

same thing here

Comment on lines +138 to +140
{appConfig.devMode && (
<Route path="components" element={<ComponentsScreen />} />
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{appConfig.devMode && (
<Route path="components" element={<ComponentsScreen />} />
)}
<Route path="components" element={<ComponentsScreen />} />

@ichub
Copy link
Contributor

ichub commented Sep 23, 2024

please run yarn in the root directory of the project to make sure that the yarn.lock is properly updated - unfortunately we need to do this anytime we install new packages in one of the subpackages of the monorepo.

@ichub
Copy link
Contributor

ichub commented Sep 23, 2024

this PR will not be able to merged without yarn lint running successfully in the root of the repo.

Screenshot 2024-09-23 at 11 35 27 AM

looks like there are a few problems - mostly formatting related. please make sure to set up your vs-code to use the prettier configuration included in the repo, so that the code is always auto-formatted consistently with the rest of the project.

@17Amir17
Copy link
Collaborator

Closed in favor of #1894

@17Amir17 17Amir17 closed this Sep 24, 2024
github-merge-queue bot pushed a commit that referenced this pull request Sep 27, 2024
copied from #1890

---------

Co-authored-by: or orsatti <[email protected]>
Co-authored-by: Amir Angel <[email protected]>
Co-authored-by: GuySerfaty <[email protected]>
rrrliu pushed a commit that referenced this pull request Sep 27, 2024
copied from #1890

---------

Co-authored-by: or orsatti <[email protected]>
Co-authored-by: Amir Angel <[email protected]>
Co-authored-by: GuySerfaty <[email protected]>
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.

4 participants