-
Notifications
You must be signed in to change notification settings - Fork 48
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
Replace Datasets DatasetSelect component on groups page with ListResources [#870] #904
Conversation
static-site/src/components/ListResources/IndividualResource.tsx
Outdated
Show resolved
Hide resolved
bc73c3b
to
a128f4b
Compare
a128f4b
to
ed27afb
Compare
ed27afb
to
18acac1
Compare
18acac1
to
b6a4195
Compare
b6a4195
to
1fb7394
Compare
Note: |
This blockage doesn't seem necessary/productive. Attempting removal with #909 |
it's not necessary, in that i could add lines to disable the couple lint failures -- but then those would need to be removed once #906 lands, and that feels like unproductive churn to me. (i don't have a strong opinion about the direction #909 moves things in; i think linting being mildly annoying when broken is maybe a good thing…) |
@genehack continuing the tangential discussion in #909 (comment) |
1fb7394
to
6f1c2d9
Compare
6f1c2d9
to
056f42e
Compare
👍 I merged #906 so that should get this unblocked soon (he says, as he discovers another linting problem...) — I will link a preview link once it builds. |
056f42e
to
89963b5
Compare
28fc179
to
bc32275
Compare
bc32275
to
7d3f74d
Compare
7d3f74d
to
691cebb
Compare
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.
The overloading of the word "group" must have made this a confusing first experience!
The error handling seems a little convoluted to me and it seems like it could be simplified in this PR, but that's the only thing that stands out to me. Functionally it's working really well.
691cebb
to
096c3fe
Compare
096c3fe
to
85ab3e7
Compare
Maybe this was addressed in a meeting off-thread you had, but since I didn't see mention of it I wanted to clarify this for you. Heroku review apps run in production mode with production config, but we've intentionally configured them (on the Heroku side) to use the |
nope, nobody else has shared this; thanks, this makes sense. |
Convert the two existing uses into the callback form. This does not add any new functionality, it only re-arranges implementation of existing functionality.
…urces [#870] * make the Resource.lastUpdated property optional * conditionalize display of "List known update" in IndividualResource TooltipWrapper * conditionalize display of "Most recent snapshot" in ResourceGroup * conditionalize use of Showcase in ListResources based on length of showcaseCards prop * conditionalize use of SortOptions in ListResources based on initial group lacking a lastUpdated field * extend groups page with ListResources callback to hit charon/getAvailableResources endpoint
out of scope for the main thrust of the branch, but quiets the typechecker.
85ab3e7
to
078e8bf
Compare
Makes sense that this is just showing blab on the preview app, but the wider blab dataset list is a bit funny in terms of collapsing. The amount of content shown should be relative to the how many lines there are. Currently the blab dataset list is definitely too long even in collapsed view. Ie collapsed height should be consistent across cards regardless of whether specific cards use one column, two columns or three columns. |
Digging ever further, this seems to be intentional behavior. @jameshadfield could you speak to this? I'm going to say I don't think this should be something that blocks this PR; if we do want to change this behavior, it can be a distinct thing. |
The intention is that if few cards are in view (commonly achieved via filtering) then we should show more entries per card. The exact thresholds are (as always) open to change. We could (probably should) also remove the expand/contract toggle if there is only 1 card. |
Description of proposed changes
Replaces the current "datasets" DatasetSelect component with a ListResources component that hits the same backend
charon
API. Also refactors inuseDataFetch
to change the hard-coded parser into a passed-in (mandatory) callback function, and updates two existing uses to the new pattern. Finally, adds in conditional display logic to the ListResources component to hide or otherwise not set UI elements that don't make sense with thegroups
dataset (e.g., last modified dates, since we don't have those).Preview
Related issue(s)
#870
Checklist