-
Notifications
You must be signed in to change notification settings - Fork 17
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 User to contain pill logic and a classes object. #235
Conversation
Deploy preview for gazebo ready! Built with commit da8f372 |
Codecov Report
@@ Coverage Diff @@
## main #235 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 74 74
Lines 553 551 -2
=========================================
- Hits 553 551 -2
Continue to review full report at Codecov.
|
student={user.student} | ||
email={user.email} | ||
isAdmin={user.isAdmin} |
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.
we could pass user={user}
instead of passing all the user information one by one
src/ui/User/User.js
Outdated
function createUserPills({ student, isAdmin, email }) { | ||
const pills = [] | ||
|
||
if (isAdmin) pills.push({ text: 'Admin', highlight: true }) | ||
if (student) pills.push({ text: 'Student' }) | ||
if (email) pills.push({ text: email }) | ||
|
||
return pills | ||
} |
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.
For the page to manage the admins, I only need to display the pill for the email. I could either not pass the student / isAdmin
as prop, but from my previous comment, I suggested to pass the full user object, so it's a bit contradictory.
I think having the pills as prop could give us the greatest flexibility in that case. Or maybe a list of pill we would like to show? pills=['student', 'admin', 'email']
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.
Problem is some pills can be highlighted / different colors. So a simple array was my initial strategy but doesn't work out for all the designs.... Though I think I just thought of a way to do this elegantly. An array that accepts strings or an object with data...
* Update customer intent * adjust to a link * Update default org * Lets move default org * Update with const to enums * redirect to landing page * Update with tests * update to move check to use user access gate * fix: Quick patch for the text alignment (#2603) * Upate with learn more link (#2604) * feat: Add in new formatTimeToString function handling milliseconds (#2600) Add in a new formatTimeToString function that handles milliseconds instead of seconds. GH codecov/engineering-team#1200 * Show next billing date for active accounts in callout view (#2585) * feat: Display list of bundles on head when comparison fails (#2598) When no comparison is present display the head commit bundle details on the PR page. * feat: #235 - Org page CTA Button Update (#2602) * update configure button, fix this prettier file? * convert to TS, add a TS-ignore for applink shenanigans * add new variant class on A tag for configure styling * fix local UTs, add a few new ones for the InactiveRepo component * update spec and small resync styling tweaks (#2609) * Update with link change (#2582) * Just update to use proper links from use Static navs * Update spelling --------- Co-authored-by: nicholas-codecov <nicholas.deschenes@sentry.io> Co-authored-by: Rohit Vinnakota <148245014+rohitvinnakota-codecov@users.noreply.github.com> Co-authored-by: ajay-sentry <159853603+ajay-sentry@users.noreply.github.com>
* Update customer intent * adjust to a link * Update default org * Lets move default org * Update with const to enums * redirect to landing page * Update with tests * update to move check to use user access gate * fix: Quick patch for the text alignment (#2603) * Upate with learn more link (#2604) * feat: Add in new formatTimeToString function handling milliseconds (#2600) Add in a new formatTimeToString function that handles milliseconds instead of seconds. GH codecov/engineering-team#1200 * Show next billing date for active accounts in callout view (#2585) * feat: Display list of bundles on head when comparison fails (#2598) When no comparison is present display the head commit bundle details on the PR page. * feat: #235 - Org page CTA Button Update (#2602) * update configure button, fix this prettier file? * convert to TS, add a TS-ignore for applink shenanigans * add new variant class on A tag for configure styling * fix local UTs, add a few new ones for the InactiveRepo component * update spec and small resync styling tweaks (#2609) * Update with link change (#2582) * Just update to use proper links from use Static navs * Update spelling --------- Co-authored-by: nicholas-codecov <nicholas.deschenes@sentry.io> Co-authored-by: Rohit Vinnakota <148245014+rohitvinnakota-codecov@users.noreply.github.com> Co-authored-by: ajay-sentry <159853603+ajay-sentry@users.noreply.github.com>
Description
Refactor User API
Added User story.
Of Note
Wanted folow up work: Pill and Avatar UI component. I think we should make a variant of avatar to control the size of the circle for more flexibility in this component (and the header).
Screenshots