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

UI library implementation #89

Merged
merged 93 commits into from
Jun 5, 2024
Merged

UI library implementation #89

merged 93 commits into from
Jun 5, 2024

Conversation

elevchyt
Copy link
Contributor

@elevchyt elevchyt commented May 31, 2024

Description

This is the new UI (aka web2).
It uses Mantine as its UI library for basic components, charts, forms & simple transitions.
Usage of route handlers & server actions has been implemented to handle data fetching/sending.

Known issues are:

  • Language picker doesn't update its dropdown value (language/locale picking works as expected, though)
  • Global (or parent) state must be implemented to communicate insights filtering changes between components like group-filters & order-filters
  • Forgot password user feedback improvements (expired tokens etc.)
  • Auth forms need improved button interactions (enabled/disabled state can be a little buggy)
  • CPM sorting hasn't been tested in the FE because of something broken in the BE when navigating to insights?orderBy=cpm

TODO:

  • Tailwind installation for more custom stylings (if needed)
  • Optional photoUrl from login.graphql (needs rebase)
  • (Maybe) Framer Motion for customized animations
  • (Maybe) Migrate from Webpack to Vite
  • Add testing library (Jest or Vitest if we migrate to Vite)
  • Handle integration errors with user feedback
  • Integration cards must use useTransition hooks
  • Use server component for all auth forms and every auth-related form should be a child of that.
  • Add login providers to sign up page, too
  • Remove iframe deprecated attributes when we add Tailwind
  • Set insight rank based on CPM
  • Update integration card on change without route refresh
  • Integration card load async
  • Bring iframe per insight on demand w/ adId

Copy link
Collaborator

@trixobird trixobird left a comment

Choose a reason for hiding this comment

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

UI look amazing, lots of code lots of suggestions :) happy to talk whenever you want

apps/web2/src/middleware.ts Outdated Show resolved Hide resolved
apps/web2/src/assets/images/linkedin-logo.png Outdated Show resolved Hide resolved
apps/web2/package.json Outdated Show resolved Hide resolved
aws/README.md Outdated Show resolved Hide resolved
apps/web2/.eslintrc.cjs Outdated Show resolved Hide resolved

const handleRevoke = (): void => {
void deAuthIntegration(props.integrationType).then(() => {
router.refresh();
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems excess, i am using a const [cardStatus, setCardStatus] = useState<IntegrationStatus>(status); can't we do the same? unless the router.refresh is super cheap and refreshes only this card. Event then I would go with a state, but you know better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it was an easy to quickly make this work.
Added to the todo.

}, 0);
}, []);

// (backend response probably needs refactor to return object with IntegrationType as key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

happy to oblige, let me know what do you want :)

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 right, this is because integrations are not dynamic (like insights) so instead of using stuff from the response's array like integrations[0], integrations[1] etc. or creating a map of integrations in the FE's code we should probably get that map from the response.

e.g.

{
 "meta": {
    id: 1,
    .
    .
  },
  "tiktok": {
    id: 2,
    .
    .
  }
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

one think that I did in web and I liked was that I had a suspence with fallback on the integrations. So if the user is on a slow connection it immediately paints the page with comming soon integrations and then it was repainted when the call was succeeding. You can emulate a very slow connection and see it (or I can artificially add a delay in the call and show it to you)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will look into it.

@elevchyt
Copy link
Contributor Author

elevchyt commented Jun 2, 2024

Thanks, I'll check them out later today!

Copy link
Collaborator

@trixobird trixobird left a comment

Choose a reason for hiding this comment

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

2nd pass :)

apps/web2/public/integrations/facebook-logo-icon.svg Outdated Show resolved Hide resolved
aws/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

it is still there I believe :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

there are still there I believe

{props.loginProviders.length ? (
<Button
component={Link}
href={props.loginProviders[0].url}
Copy link
Collaborator

Choose a reason for hiding this comment

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

that's a bit dangerous. It couples FE/BE. What will happen if on the backend I add a new provider and I return the new one first?

Copy link
Collaborator

@trixobird trixobird left a comment

Choose a reason for hiding this comment

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

3rd pass

apps/web2/src/app/(authenticated)/layout.tsx Outdated Show resolved Hide resolved

export default function RootLayout({ children }: { children: React.ReactNode }): React.ReactNode {
return (
<html lang="en">
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should be access this though the cookie. Also right now the correct value is en-gb, although I think we need to decouple LOCALE and Lang

apps/web2/package.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@trixobird trixobird left a comment

Choose a reason for hiding this comment

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

hype!!

@elevchyt elevchyt merged commit 4488fd6 into main Jun 5, 2024
4 checks passed
@elevchyt elevchyt deleted the ui-library-implementation branch June 5, 2024 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants