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

Containers refactor III - This time its composable #5715

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

pete-watters
Copy link
Contributor

@pete-watters pete-watters commented Jul 31, 2024

Try out Leather build d804675Extension build, Test report, Storybook, Chromatic

This PR addresses some concerns around how I implemented adding global containers. See discussion here.

There is a lot of code here but the main update is:

  • use of routes to configure app/features/container is now gone
  • Pages get have to opt in and configure their own header - <MainHeader, <PageHeader, <PopupHeader
  • <Home pages have this set @ app-route level as they share a simpler header
  • Other full view pages use the <Page wrapper to control their width:
<Container>
    <PageHeader title="Send" />
    <ContainerLayout>
          <Content>
                <Page>
                </Page>
          </Content>
      </ContainerLayout>    
   </Container>     
  • <Popup pages are popup windows for Approval UX and don't need the <Page wrapper

This is an improvement but could probably be improved more so please let me know of any suggestions.

@pete-watters pete-watters force-pushed the chore/refactor-containers branch 3 times, most recently from ba132c4 to 7d78a08 Compare August 1, 2024 10:17
@markmhendrickson
Copy link
Collaborator

Thanks for taking this initiative, @pete-watters. Is it worth getting some reviewer eyes on this now for feedback, despite it being a draft PR still?

@pete-watters
Copy link
Contributor Author

Thanks for taking this initiative, @pete-watters. Is it worth getting some reviewer eyes on this now for feedback, despite it being a draft PR still?

I'm working on a final fix for Popup's then I will open for full review in the next hour no matter what 👍

@pete-watters pete-watters force-pushed the chore/refactor-containers branch from 21b14c4 to 2e39ebd Compare August 1, 2024 12:44
@pete-watters
Copy link
Contributor Author

Thanks for taking this initiative, @pete-watters. Is it worth getting some reviewer eyes on this now for feedback, despite it being a draft PR still?

I'm working on a final fix for Popup's then I will open for full review in the next hour no matter what 👍

OK it turns out everything was working and the problem was solved by a lunch break

@pete-watters pete-watters marked this pull request as ready for review August 1, 2024 12:45
@pete-watters
Copy link
Contributor Author

@kyranjamie @alter-eggo : Can you please check this PR when you have some time?

Its quite large so apologies for that but hopefully it adds a big improvement to the scalability of the container pattern

hideBackButton?: boolean;
hideLogo?: boolean;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can refactor <MainHeader and <PageHeader to merge them and keep them DRY but didn't want to let this drag on longer before review

@pete-watters pete-watters changed the title Containers refactor III Containers refactor III - This time its composable Aug 1, 2024
Copy link
Collaborator

@kyranjamie kyranjamie left a comment

Choose a reason for hiding this comment

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

Thanks Pete, this looks like a well organised, configurable solution 👍🏻

src/app/pages/onboarding/sign-in/sign-in.tsx Outdated Show resolved Hide resolved
@pete-watters pete-watters added this pull request to the merge queue Aug 1, 2024
Merged via the queue into dev with commit b5b7988 Aug 1, 2024
30 checks passed
@pete-watters pete-watters deleted the chore/refactor-containers branch August 1, 2024 15:10
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.

3 participants