-
Notifications
You must be signed in to change notification settings - Fork 3
UAI Dashboard #2189
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
UAI Dashboard #2189
Conversation
OpenAPI ChangesShow/hide No detectable change.
|
OpenAPI ChangesShow/hide No detectable change.
|
OpenAPI ChangesShow/hide No detectable change.
|
19e058f
to
00388cc
Compare
OpenAPI ChangesShow/hide No detectable change.
|
OpenAPI ChangesShow/hide No detectable change.
|
<Stack | ||
direction="row" | ||
justifyContent="space-between" | ||
alignItems="stretch" | ||
flex={1} | ||
width="100%" | ||
> | ||
<Stack direction="column" gap="8px"> | ||
<Stack direction="column" gap="8px" flex={1}> |
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.
<CardRoot | ||
screenSize="desktop" | ||
data-testid="enrollment-card-desktop" | ||
as={Component} | ||
className={className} | ||
> |
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.
@gumaerc I moved the desktop-only / mobile-only behavior to CardRoot.
My motivation was:
- I want
Component
to be the root component (So that if we nest cards in a list, we can specifyComponent="li"
- Classname should similarly apply to the root component.
Alternatively, could have moved classname, testid, and Component onto DesktopOnly
/ MobileOnly
. Not sure what's more preferable.
@@ -33,7 +33,7 @@ const someAncestor = (el: HTMLElement, cb: (el: HTMLElement) => boolean) => { | |||
} | |||
|
|||
const setupApis = ( | |||
channelPatch?: Partial<Channel>, | |||
channelPatch: Partial<Channel> = {}, |
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.
This is related to the change to PartialFactory that I made. Since PartialFactory now accepts an array of overrides, explicitly passing undefined is no longer allowed. I.e.,
partialFactory()
OK ✅partialFactory(undefined)
❌ not ok
describe.each([ | ||
{ display: "desktop", testId: "enrollment-card-desktop" }, | ||
{ display: "mobile", testId: "enrollment-card-mobile" }, | ||
])("EnrollmentCard $display", ({ testId }) => { |
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.
@gumaerc I did decide to parameterize the desktop vs mobile testing. I'm worried without this we'll forget to test both cases in the future.
(Not that the possibility of forgetting is gone now. You could still ignore the parameterized testId.)
* | ||
* The query string is sorted by key. | ||
*/ | ||
const queryify = (params: unknown, { explode = true } = {}) => { |
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.
This function is moved from api/src/test-utils/index.ts
. The only change was addition of explode
option, since mitxonline does not explode query parameters.
const mobileCards = await screen.findAllByTestId("enrollment-card-mobile") | ||
expect(desktopCards.length).toBe(7) | ||
expect(mobileCards.length).toBe(7) | ||
const cards = await screen.findAllByTestId("enrollment-card-desktop") |
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.
Removed the desktop / mobile distinction, since that seems like an implementation detail of DashboardCard to me.
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.
👍 LGTM
I just noticed a few typos in some comments:
<ImageContainer> | ||
<Image | ||
width={72} | ||
// NextJS Image component requires width and height to avoid loayout shift. |
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.
"layout shift"
// NextJS Image component requires width and height to avoid loayout shift. | ||
// These are supposed to be the intrinsic sizes of the image file. | ||
// That said, it's not particularly relevant here since the parent is | ||
// reserving spae for the image anyway. Using next/image still gets us |
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.
"space"
OpenAPI ChangesShow/hide No detectable change.
|
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/7004
Description (What does it do?)
This adds the Universal AI org-level dashboard.
It does NOT include:
Forthcoming APIs This PR makes some assumptions about forthcoming APIs, based on conversations with @jkachel. It would be good to document these assumptions somewhere. In particular, the assumptions are:
{ id, name, logo }
.org_id
filter. (OrorgId
once camelized by our api client generator.)Screenshots (if appropriate):
How can this be tested?
mitlearn-organization-dashboard
OFF:enrollment-dashboard
flag onmitlearn-organization-dashboard
ON:Before Merging