-
Notifications
You must be signed in to change notification settings - Fork 2
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
refactor: Set up layout and styling for team page #4172
refactor: Set up layout and styling for team page #4172
Conversation
Removed vultr server and associated DNS entries |
@ianjon3s Is there a PR for |
@DafyddLlyr currently the branch is empty/unchanged so I couldn't create a PR with it. The intention is to split each piece of work from #4120 into a smaller PR, and merge them into |
Perfect! Sounds like a solid plan to - thanks for the explanation 👍 Will review this one later this afternoon. |
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.
Looking great and working as expected - a few very minor comments.
<InputRowLabel> | ||
<strong>Search</strong> | ||
</InputRowLabel> | ||
<InputRowItem> | ||
<Box sx={{ position: "relative" }}> | ||
<Input | ||
sx={{ | ||
borderColor: (theme) => theme.palette.border.input, | ||
pr: 5, | ||
}} | ||
name="search" | ||
id="search" | ||
/> | ||
</Box> | ||
</InputRowItem> |
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.
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.
I've moved this to it's own component in this PR, if this is worth adding here: #4174
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.
To keep things simple I'd wait until this is merged to the main feature branch and use that as base to build onto.
editor.planx.uk/src/pages/Team.tsx
Outdated
const DashboardLink = styled(Link)(({ theme }) => ({ | ||
display: "block", | ||
fontSize: theme.typography.h4.fontSize, | ||
const FlowCardContent = styled(Box)(({ theme }) => ({ |
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.
I'd recommend splitting up this file and adding a few additional files for these now components. There's a whole lot going on here already - it was probably too big before this PR in all honesty.
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.
Great shout, I'll create a dedicated component file for FlowCard
to get started.
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.
I have now split FlowCard
into its own component. Something I'm not sure about:
- I'm using the styling of the card as a 'shell' for a dummy card that displays when no flows have been created. In order to do this I've imported the styling only for
Card
andCardContent
for use inTeam.tsx
. Is this considered a good approach?
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.
Yep - this change looks great and no issues with this approach.
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.
Great! One nit, this can be picked up here or in a follow up PR 👌
<Box sx={{ display: "flex", alignItems: "center", gap: 0.2 }}> | ||
<MoreHoriz sx={{ fontSize: "1.4em" }} /> | ||
<Typography variant="body2" fontSize="small"> | ||
<strong>Menu</strong> | ||
</Typography> | ||
</Box> |
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.
nit: Not sure exactly where it's getting applied, but there's a MUI ripple here we usually remove with the disableRipple
prop
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.
Ah yes good spot, that'll be coming from SimpleMenu
, will amend
Cleared caches and re-ran CI here - all passing now! ✅ |
Thanks for this @DafyddLlyr ! |
What does this PR do?
Refactors layout and styling for the updated team page:
Preview:
https://4172.planx.pizza/buckinghamshire