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

My Libraries #336

Merged
merged 39 commits into from
Nov 2, 2023
Merged

My Libraries #336

merged 39 commits into from
Nov 2, 2023

Conversation

shinyichen
Copy link
Member

@shinyichen shinyichen commented Oct 25, 2023

All the existing libraries implementations

src/api/biblib/types.ts Outdated Show resolved Hide resolved
@shinyichen shinyichen marked this pull request as ready for review October 27, 2023 18:49
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #336 (9add16f) into master (590c81c) will decrease coverage by 0.33%.
Report is 2 commits behind head on master.
The diff coverage is 44.33%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #336      +/-   ##
==========================================
- Coverage   48.14%   47.81%   -0.33%     
==========================================
  Files         369      389      +20     
  Lines       37034    40442    +3408     
  Branches      511      554      +43     
==========================================
+ Hits        17829    19337    +1508     
- Misses      19196    21093    +1897     
- Partials        9       12       +3     
Files Coverage Δ
src/api/biblib/index.ts 100.00% <100.00%> (ø)
src/api/biblib/types.ts 100.00% <100.00%> (ø)
src/api/models.ts 100.00% <100.00%> (ø)
src/api/search/types.ts 100.00% <100.00%> (ø)
src/components/Libraries/TableSkeleton.tsx 100.00% <100.00%> (ø)
src/components/Libraries/index.ts 100.00% <100.00%> (ø)
src/components/Libraries/types.ts 100.00% <100.00%> (ø)
src/components/Pagination/index.ts 100.00% <100.00%> (ø)
src/components/ResultList/useHighlights.ts 86.20% <100.00%> (ø)
src/components/SearchBar/SearchInput.tsx 82.57% <ø> (-0.13%) ⬇️
... and 24 more

... and 2 files with indirect coverage changes

@thostetler
Copy link
Member

image
I think we should hide the pagination controls and operations when no libraries are found.

@thostetler
Copy link
Member

thostetler commented Oct 31, 2023

image
should lowercase the 'a'


image
I think we could have a little more vertical margin, feels a little closed in when you have a low amount of libs


image
We could probably keep the modal open and let the user know that the name they chose was already taken. I'm thinking of a situation where I entered the wrong name, but wrote out a long description -- currently I'd lose what I wrote...


https://github.com/adsabs/nectar/assets/6970899/5b2d92e9-539e-4702-a4f2-614027cba5a3
The back button on a paper details page doesn't go back to main libraries page as I would expect


image
Seems like collaborators are slightly off center


image
I think I would prefer the modal to be bigger so the library list isn't quite so tight


https://github.com/adsabs/nectar/assets/6970899/17aaa5c3-a587-486c-9089-aa62d139cafc
Seeing a bug where after adding a new library it's not showing up in the list


image
Should use humanized date strings, similar to how we have it in the ORCiD works table:

const getUpdated = (date: string) => {


image
On library details page, when request is blocked (error state) the page remains in a loading state


image
on the main libraries table, if biblib requests are blocked, the page throws an error


image
I'm not clear on the purpose of the dropdown at the top.


https://github.com/adsabs/nectar/assets/6970899/130067b3-f4bb-4b62-8829-46c2e637265c
Deleting a library from settings does not update the main library list.


Other small nits:

  • In the modals that are in use, we should place focus into whatever primary input/button

Copy link
Member

@thostetler thostetler left a comment

Choose a reason for hiding this comment

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

Nice work, everything works well and feels natural coming from BBB. I have a few things I noticed, but no show stoppers.
LGTM

src/components/Libraries/LibraryListTable.tsx Show resolved Hide resolved
src/components/Libraries/LibrarySelector.tsx Outdated Show resolved Hide resolved
Comment on lines 160 to 162
<SimpleLink href={`/user/libraries/${id}`} display="inline">
<ChevronLeftIcon mr={2} />
</SimpleLink>
Copy link
Member

Choose a reason for hiding this comment

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

We should have a component for these back links. This should work like a stack that we 'push' as we dig in, and 'pop' as we go back up

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the advantage for the component? I can add a task for this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah we could think about it anyway, we should probably use hash urls or something like that as well for the sub-views so that we could keep track of it/shareable/etc.

src/components/Libraries/OperationModal.tsx Outdated Show resolved Hide resolved
src/pages/user/libraries/[[...id]].tsx Outdated Show resolved Hide resolved
src/pages/user/libraries/[[...id]].tsx Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
@shinyichen
Copy link
Member Author

image I think we should hide the pagination controls and operations when no libraries are found.

Resolved

@shinyichen
Copy link
Member Author

Resolved.

The drop down is a placeholder where we will have 'my libraries', 'shared with me' and 'following' in the future.

image should lowercase the 'a'

image I think we could have a little more vertical margin, feels a little closed in when you have a low amount of libs

image We could probably keep the modal open and let the user know that the name they chose was already taken. I'm thinking of a situation where I entered the wrong name, but wrote out a long description -- currently I'd lose what I wrote...

https://github.com/adsabs/nectar/assets/6970899/5b2d92e9-539e-4702-a4f2-614027cba5a3 The back button on a paper details page doesn't go back to main libraries page as I would expect

image Seems like collaborators are slightly off center

image I think I would prefer the modal to be bigger so the library list isn't quite so tight

https://github.com/adsabs/nectar/assets/6970899/17aaa5c3-a587-486c-9089-aa62d139cafc Seeing a bug where after adding a new library it's not showing up in the list

image Should use humanized date strings, similar to how we have it in the ORCiD works table:

const getUpdated = (date: string) => {

image On library details page, when request is blocked (error state) the page remains in a loading state

image on the main libraries table, if biblib requests are blocked, the page throws an error

image I'm not clear on the purpose of the dropdown at the top.

https://github.com/adsabs/nectar/assets/6970899/130067b3-f4bb-4b62-8829-46c2e637265c Deleting a library from settings does not update the main library list.

Other small nits:

  • In the modals that are in use, we should place focus into whatever primary input/button

image should lowercase the 'a'

image I think we could have a little more vertical margin, feels a little closed in when you have a low amount of libs

image We could probably keep the modal open and let the user know that the name they chose was already taken. I'm thinking of a situation where I entered the wrong name, but wrote out a long description -- currently I'd lose what I wrote...

https://github.com/adsabs/nectar/assets/6970899/5b2d92e9-539e-4702-a4f2-614027cba5a3 The back button on a paper details page doesn't go back to main libraries page as I would expect

image Seems like collaborators are slightly off center

image I think I would prefer the modal to be bigger so the library list isn't quite so tight

https://github.com/adsabs/nectar/assets/6970899/17aaa5c3-a587-486c-9089-aa62d139cafc Seeing a bug where after adding a new library it's not showing up in the list

image Should use humanized date strings, similar to how we have it in the ORCiD works table:

const getUpdated = (date: string) => {

image On library details page, when request is blocked (error state) the page remains in a loading state

image on the main libraries table, if biblib requests are blocked, the page throws an error

image I'm not clear on the purpose of the dropdown at the top.

https://github.com/adsabs/nectar/assets/6970899/130067b3-f4bb-4b62-8829-46c2e637265c Deleting a library from settings does not update the main library list.

Other small nits:

  • In the modals that are in use, we should place focus into whatever primary input/button

@shinyichen
Copy link
Member Author

@thostetler All the items are resolved. One item with the linking I think can be a separate task.

@shinyichen shinyichen merged commit b9ea756 into adsabs:master Nov 2, 2023
3 of 4 checks passed
@shinyichen shinyichen deleted the mylibraries branch December 6, 2023 00:05
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.

2 participants