-
Notifications
You must be signed in to change notification settings - Fork 34
feat: Add org list for multi org setups on plan page #3407
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
Conversation
| const variables = { | ||
| first, | ||
| ordering, | ||
| orderingDirection, |
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.
Bundle ReportChanges will increase total bundle size by 9.74kB (0.06%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3407 +/- ##
=======================================
Coverage 99.15% 99.15%
=======================================
Files 809 809
Lines 14239 14275 +36
Branches 3928 3935 +7
=======================================
+ Hits 14118 14154 +36
Misses 112 112
Partials 9 9
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3407 +/- ##
=======================================
Coverage 99.15% 99.15%
=======================================
Files 809 809
Lines 14239 14275 +36
Branches 3923 3935 +12
=======================================
+ Hits 14118 14154 +36
Misses 112 112
Partials 9 9
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3407 +/- ##
=======================================
Coverage 99.15% 99.15%
=======================================
Files 809 809
Lines 14239 14275 +36
Branches 3923 3942 +19
=======================================
+ Hits 14118 14154 +36
Misses 112 112
Partials 9 9
Continue to review full report in Codecov by Sentry.
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #3407 +/- ##
=======================================
Coverage 99.15% 99.15%
=======================================
Files 809 809
Lines 14239 14275 +36
Branches 3923 3942 +19
=======================================
+ Hits 14118 14154 +36
Misses 112 112
Partials 9 9
Continue to review full report in Codecov by Sentry.
|
Bundle ReportChanges will increase total bundle size by 9.74kB (0.06%) ⬆️. This is within the configured threshold ✅ Detailed changes
|
nicholas-codecov
left a comment
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.
couple of comments to take a peek at
| <th | ||
| key={header.id} | ||
| scope="col" | ||
| data-sortable={header.column.getCanSort()} | ||
| onClick={header.column.getToggleSortingHandler()} | ||
| > | ||
| <div className="flex"> | ||
| {flexRender( | ||
| header.column.columnDef.header, | ||
| header.getContext() | ||
| )} | ||
| <span | ||
| className="text-ds-blue-darker group-hover/columnheader:opacity-100" | ||
| data-sort-direction={header.column.getIsSorted()} | ||
| > | ||
| <Icon name="arrowUp" size="sm" /> | ||
| </span> | ||
| </div> | ||
| </th> |
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.
Has cursor:pointer been disabled on non-sortable headers? I don't see anything explicit here tho it may be somewhere else, and I don't think we have a test org on staging to check this.
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.
Codecov org works for testing. I set the sortability on the column definitions see line 82.
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.
Ahh okay, was just making sure that the styles were being applied correctly when they're set to false, everything looks good 👍
| <tr | ||
| key={row.id} | ||
| className="h-14 hover:cursor-pointer hover:bg-ds-gray-primary" | ||
| onClick={() => linkToMembersTab(row.original.name)} |
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.
Instead of using an onClick handler here, are we able to make the org name an anchor tag?
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 think this could probably be done up in the column declarations
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 want the whole row to be a button, not just a single column. Tried wrapping the whole thing in anchor and also played around with Applink/NavLink/A. This was the best I could find for achieving what we want. The one thing I think this is missing is proper a11y treatment, I will add this.
2ea8748 to
90ff363
Compare
| <div className="flex w-full justify-end"> | ||
| {/* @ts-ignore-error */} | ||
| <A | ||
| to={{ | ||
| pageName: 'membersTab', | ||
| options: { | ||
| owner: encodeURIComponent( | ||
| row.original.name | ||
| ), | ||
| provider, | ||
| }, | ||
| }} | ||
| > | ||
| {cell.getValue()} | ||
| </A> | ||
| </div> |
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.
Thoughts on bumping this up into the cell function for this column?
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 considered this, but thought it wasn't possible due to dependency on provider. Do you have magic for this?
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.
You shouldn't have to provide the provider for this
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.
useNavLinks under the hood should get this value for you
Adds the last piece of this feature: the organization list!
During development we ran into some issues with the API and not having proper access, but that has been fixed over there pending my PR getting merged.
(Minor design changes have been okayed with design folks)
Design
Closes codecov/engineering-team#2564