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

refactor(site): generalize UserCell component #484

Merged
merged 7 commits into from
Mar 23, 2022

Conversation

greyscaled
Copy link
Contributor

@greyscaled greyscaled commented Mar 18, 2022

Summary:

This is a first step in porting over v1 AuditLog in a refactored/cleaned
up fashion. The existing UserCell component was generalized for re-use.

Details:

  • Move UserCell to components/Table/Cells
  • Add tests and stories for UserCell

Impact:

The UserCell is used in the User dropdown menu and in the Navbar. We should verify that
those are unaffected by this change. Furthermore, this unblocks future work in list views
like the audit log, user management panel and organizations management panel.

Relations:

TODO:

@greyscaled greyscaled self-assigned this Mar 18, 2022
@codecov
Copy link

codecov bot commented Mar 18, 2022

Codecov Report

Merging #484 (ec4ac6d) into main (26d24f4) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #484      +/-   ##
==========================================
+ Coverage   60.35%   60.42%   +0.06%     
==========================================
  Files         191      193       +2     
  Lines       10864    10878      +14     
  Branches       85       87       +2     
==========================================
+ Hits         6557     6573      +16     
+ Misses       3587     3585       -2     
  Partials      720      720              
Flag Coverage Δ
unittest-go-macos-latest 56.25% <ø> (+0.13%) ⬆️
unittest-go-ubuntu-latest 59.75% <ø> (-0.09%) ⬇️
unittest-go-windows-2022 55.89% <ø> (-0.03%) ⬇️
unittest-js 63.89% <100.00%> (+0.70%) ⬆️
Impacted Files Coverage Δ
site/src/components/Navbar/UserDropdown.tsx 86.95% <ø> (ø)
site/src/components/User/UserProfileCard.tsx 75.00% <ø> (ø)
site/src/components/Table/Cells/UserCell.tsx 100.00% <100.00%> (ø)
site/src/components/User/UserAvatar.tsx 100.00% <100.00%> (+12.50%) ⬆️
site/src/test_helpers/entities.ts 100.00% <100.00%> (ø)
site/src/util/first-letter.ts 100.00% <100.00%> (ø)
cli/config/file.go 67.64% <0.00%> (-8.83%) ⬇️
peer/conn.go 75.38% <0.00%> (-0.77%) ⬇️
provisionerd/provisionerd.go 78.03% <0.00%> (+1.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26d24f4...ec4ac6d. Read the comment docs.

Summary:

This is a first step in porting over v1 AuditLog in a refactored/cleaned
up fashion. This isn't a direct port, since we do not yet have a
UserAvatar component.

Details:

- Port over UserCell from v1, sans UserAvatar impl
- Add tests and stories for UserCell

Notes:

We do not have a holistic solution for handling localization, but
starting from some kind of easy way that collects/resources strings will
make the migration significantly easier. It will also help out our
product copy owner, @khorne3 with maintenance. An RFC regarding this
might be necessitated.

Impact:

This change does not have any user-facing impact yet, because the
UserCell is not yet rendered in the product. This enables an incremental
approach to migrating in the FE of the Audit Log, which is still waiting
on the BE port.

Relations:

- This commit relates to #472, but does not finish it.
- This commit should not merge until after #465 and #483 because it's
based on them.
@greyscaled greyscaled force-pushed the vapurrmaid/472-init-audit-log branch from 8b7a482 to 7a6df41 Compare March 18, 2022 21:48
@greyscaled greyscaled marked this pull request as ready for review March 18, 2022 21:49
@greyscaled greyscaled requested a review from a team as a code owner March 18, 2022 21:49
greyscaled added a commit that referenced this pull request Mar 19, 2022
Summary:

This is a direct follow-up to #484 and a next step in porting over the
AuditLog with appropriate refactoring. This isn't a direct port; we used
to have a map of icons on the FE, but we want to no loner split the
logic between FE and BE. If we want icons at a later time point, we can
return an icon identifier in the response, or we can simplify the view
and gain real estate by omitting the icons. At the time of this commit,
I prefer omitting the icons.

Details:

- Port over ActionCell from v1, sans icons
- Add tests and stories for ActionCell

Impact:

This change does not have any user-facing impact yet, because the
ActionCell is not yet rendered in the product. This enables an incremental
approach to migrating in the FE of the Audit Log, which is still waiting
on the BE port.

Relations:

- This commit relates to #472, but does not finish it.
- This commit should not merged until after #484 because it's based from
it
greyscaled added a commit that referenced this pull request Mar 19, 2022
Summary:

This is a direct follow-up to #484 and #500. It is a part of many steps
in porting/refactoring the AuditLog from v1.

Details:

- Port over TargetCell from v1, with refactorings
- Add tests and stories

Impact:

This change does not have any user-facing impact yet because AuditLog is
not yet available in the product. This is part of an incremental
approach; the FE is still waiting on the BE port.

Relations:

- This commit relates to #472, but does not finish it
- This commit should not be merged until after #484 and #500, because it
builds off of them.
greyscaled added a commit that referenced this pull request Mar 20, 2022
Summary:

This is direct follow-up to #484, #500, and #501. It is a part of many
steps in porting/refactoring the AuditLog from v1.

Details:

- Port over TimeCell from v1, with refactorings
- A jest test was not added because we use locale dates and times, which
can be tricky to test. This can be addressed in the future.

Impact:

This change does not have any user-facing impact yet because AuditLog is
not yet available in the product. This is part of an incremental
approach; the FE is still waiting on the BE port.

Relations:

- This commit relates to #472, but does not finish it
- This commit should not be merged until after #484, #500 and #501,
because it builds off of them
@presleyp
Copy link
Contributor

We have a UserAvatar component, in components/User. Will that one work? Makes me think we should have less nesting of our components so we're more likely to reuse them. Speaking of, can we reuse this UserCell outside of the Audit Log? I know we have user cells in Audit Log, Users, and Organization. Looks like the latter two are the same and the first just differs in what detail is in lighter text at the bottom, so maybe we could put UserCell directly in components and give it a prop that determines which detail it shows.

@greyscaled
Copy link
Contributor Author

We have a UserAvatar component, in components/User. Will that one work? Makes me think we should have less nesting of our components so we're more likely to reuse them. Speaking of, can we reuse this UserCell outside of the Audit Log? I know we have user cells in Audit Log, Users, and Organization. Looks like the latter two are the same and the first just differs in what detail is in lighter text at the bottom, so maybe we could put UserCell directly in components and give it a prop that determines which detail it shows.

Great feedback!

@greyscaled greyscaled changed the title feat(site): add UserCell component refactor(site): generalize UserCell component Mar 22, 2022
import { UserCell, UserCellProps } from "./UserCell"

export default {
title: "Table/Cells/UserCell",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@presleyp - do you like this organization (Table > Cells) ? Another possibility is a new directory "TableCells". I could go either way, just wanna find the one we all think is best.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this seems good! Having a manageable number of top level categories will be nice.

Copy link
Contributor

@presleyp presleyp left a comment

Choose a reason for hiding this comment

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

Thanks for doing the mocks and tests, looks great!

import { UserCell, UserCellProps } from "./UserCell"

export default {
title: "Table/Cells/UserCell",
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah this seems good! Having a manageable number of top level categories will be nice.

@@ -1,25 +1,12 @@
import Avatar from "@material-ui/core/Avatar"
import React from "react"
import { UserResponse } from "../../api/types"
import { firstLetter } from "../../util/first-letter"
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome!

@@ -15,7 +15,7 @@ export const UserProfileCard: React.FC<UserProfileCardProps> = ({ user }) => {
return (
<div className={styles.root}>
<div className={styles.avatarContainer}>
<UserAvatar className={styles.avatar} user={user} />
<UserAvatar className={styles.avatar} username={user.username} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Great choice

@greyscaled greyscaled merged commit 6560f2e into main Mar 23, 2022
@greyscaled greyscaled deleted the vapurrmaid/472-init-audit-log branch March 23, 2022 14:28
@misskniss misskniss added this to the V2 Beta milestone May 15, 2022
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.

None yet

3 participants