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

[Feature] Admin department page styling #11853

Merged
merged 21 commits into from
Oct 30, 2024
Merged

Conversation

petertgiles
Copy link
Contributor

@petertgiles petertgiles commented Oct 24, 2024

πŸ€– Resolves #11478

πŸ‘‹ Introduction

Updates the styling of the admin department pages and adds a new "view department" page.

πŸ•΅οΈ Details

I've rolled a few more things into this one.

I reduced the bottom padding on the new hero to x2. I've also removed the built-in navbar and x3 of top padding.
image

I created a new CardSectioned component. Visually it is based on CardBasic and it is broken up like PreviewList. It's not very flexible right now because we didn't receive any designs for it. Hopefully it can become a standard component that is reused widely someday.

image

πŸ§ͺ Testing

  1. Rebuild and log in as admin
  2. View the department pages
  • Mobile
  • Desktop

πŸ“Έ Screenshot

image

Copy link
Contributor

@mnigh mnigh left a comment

Choose a reason for hiding this comment

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

Some comments related to copy.

}),
}}
add={{
linkProps: {
href: paths.departmentCreate(),
label: intl.formatMessage({
defaultMessage: "Create Department",
id: "ZbpbD6",
defaultMessage: "Create new department",
Copy link
Contributor

Choose a reason for hiding this comment

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

New is implied when creating something. Can we remove the word new?

columnHelper.accessor("departmentNumber", {
id: "departmentNumber",
filterFn: "weakEquals",
header: intl.formatMessage({
defaultMessage: "Department #",
id: "QOvS1b",
defaultMessage: "Number",
Copy link
Contributor

Choose a reason for hiding this comment

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

Everywhere else in the forms, it is referred to as Department number. Should it not be kept the same for consistency?

defaultMessage: "Search departments",
id: "bUyxJi",
description: "Label for the departments table search input",
defaultMessage: "Search by keyword",
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a keyword?

defaultMessage: 'Use the "Create Department" button to get started.',
id: "yat9wx",
defaultMessage:
'Use the "Create new department" button to get started.',
Copy link
Contributor

Choose a reason for hiding this comment

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

@yonikid15 yonikid15 self-requested a review October 29, 2024 15:02
Copy link
Contributor

@yonikid15 yonikid15 left a comment

Choose a reason for hiding this comment

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

Nice work as always πŸŽ‰
For some reason the nav menu's z-index is overlapping the overlay only on the edit page, when the page is loading πŸ˜• (why isn't it happening on the other pages too πŸ˜†). To fix this, we can create a new issue or just set them to "2" in lines NavigationMenu-L45 and NavigationMenu-L184

Screenshot from 2024-10-29 14-34-27

Comment on lines +240 to +244
const departmentName = getLocalizedName(
departmentData?.department?.name,
intl,
);

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm seeing a flash of "Not available" in the Hero title, while the department is being fetched. This is out of scope , and not blocking this, so I'll bring it up to the designers, to see what a good solution is.

Screenshot from 2024-10-29 14-38-12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good catch. I could have this be an empty string or "Loading" or something if 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.

OK, I added the loading string
loading title for view page

@yonikid15
Copy link
Contributor

yonikid15 commented Oct 29, 2024

Also the sign out and notifications buttons dark/light modes, aren't synced up with the rest of the dialog. (See far right of nav menu in image in light mode).
image

Again, this is out of scope, but seems like an easy fix. If not I can create a new issue. Otherwise the design and code looks good πŸ‘

@petertgiles
Copy link
Contributor Author

petertgiles commented Oct 30, 2024

As discussed with Josh, I just removed the navbar that was built-in to the hero and x3 of top padding. This should sidestep most of the issues you noticed.
remove hero nav and padding

@mnigh mnigh changed the title [Feature] Admin depertment page styling [Feature] Admin department page styling Oct 30, 2024
@petertgiles petertgiles added this pull request to the merge queue Oct 30, 2024
Merged via the queue into main with commit a910733 Oct 30, 2024
10 of 11 checks passed
@petertgiles petertgiles deleted the 11478-admin-departments-styling branch October 30, 2024 17:22
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.

✨ Admin / Departments / mostly Styling update
3 participants