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

feat: #569 implement the redesign for dashboard and side nav #613

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

craigyu
Copy link
Collaborator

@craigyu craigyu commented Feb 11, 2025

Description

Implements #569

Implemented the new design for dashboard and and side nav

  • removed some unused components and test files
  • refactored styles to use proper grid in related components
  • refactored how backend data is handled
  • introduced 2 new packages
    1. Tanstack query dev tool for improved QoL
    2. luxon for easy date formatting

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

ick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc... -->


Thanks for the PR!

Deployments, as required, will be available below:

Please create PRs in draft mode. Mark as ready to enable:

After merge, new images are deployed in:


Thanks for the PR!

Deployments, as required, will be available below:

Please create PRs in draft mode. Mark as ready to enable:

After merge, new images are deployed in:

@craigyu craigyu self-assigned this Feb 11, 2025
@craigyu craigyu linked an issue Feb 11, 2025 that may be closed by this pull request
@craigyu craigyu changed the title Feat/569 implement the redesign for dashboard and side nav feat: #569 implement the redesign for dashboard and side nav Feb 11, 2025
Copy link
Contributor

@paulushcgcj paulushcgcj left a comment

Choose a reason for hiding this comment

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

One suggestion I would give is to start replacing the imports to use the path definition set on the tsconfig and vite config files to avoid path navigations and possibly issues when importing or copying imports from other files.

@@ -46,6 +48,33 @@ const protectedRoutes: RouteObject[] = [
}
];

// Tanstack Query Config
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this into a separate file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

7d6cfd8

done

min-width: 1rem;
min-height: 1rem;
}
.divisory {
Copy link
Contributor

Choose a reason for hiding this comment

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

As the hr tag was removed from the template file, I think is safe to remove this css class from here as it is not being required anymore

Copy link
Collaborator Author

@craigyu craigyu Feb 15, 2025

Choose a reason for hiding this comment

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

no there's still an hr element at line 48 😛


.chart-holder {

.cds--cc--axes g.axis g.tick text {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we use the vars.$bcgov-prefix variable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the vars.$bcgov-prefix is 'bx', i think the prefix here is special for the chart component, honestly i didn't find this css having any effects, but left it here from the previous code

{
!recentOpeningsQuery.isLoading && recentOpeningsQuery.data?.data.length ?
(
<Table
Copy link
Contributor

Choose a reason for hiding this comment

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

One suggestion here would be to try to generify this table, something like this I am doing on another PR.

With a few changes on the headers object, we can even set if a specific field should use a component to render (such as the date, status, truncated texts). This would reduce code duplication, as we can just provide to the data table the page content, pagination content and possible empty scenario flags/texts

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah that's a good idea, it might be better to do it in a separate issue, then we can have all tables to use the generic table with loading and error handling

@craigyu craigyu marked this pull request as ready for review February 15, 2025 01:55
@craigyu craigyu requested a review from paulushcgcj February 15, 2025 01:55
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.

Implement the redesign for dashboard and side nav
2 participants