feat: Add hover states to bookmarks #987
Conversation
|
This pull request has been linked to Shortcut Story #1568: Add hover states to bookmarks. |
| <svg | ||
| width="20" | ||
| height="20" | ||
| viewBox="0 0 20 20" | ||
| fill="none" | ||
| xmlns="http://www.w3.org/2000/svg"> | ||
| <path | ||
| fillRule="evenodd" | ||
| clipRule="evenodd" | ||
| d="M10 0C4.48 0 0 4.48 0 10C0 15.52 4.48 20 10 20C15.52 20 20 15.52 20 10C20 4.48 15.52 0 10 0ZM11 15H9V9H11V15ZM11 7H9V5H11V7Z" | ||
| fill="currentColor" | ||
| /> | ||
| </svg> |
There was a problem hiding this comment.
Added this inline because we can change the fill value to currentColor and then control the color with CSS, as opposed to having to add a second icon for dark theme.
jcbcapps
changed the title
|
the info icon is not currently available to keyboard users |
…ssf-portal-client into sc-1568/bookmark-hover-info
| @@ -63,8 +63,8 @@ const Error: NextPageWithLayout<Props> = ({ statusCode }: Props) => { | |||
| ) | |||
| } | |||
|
|
|||
| Error.getInitialProps = ({ res, err }: NextPageContext) => { | |||
| const statusCode = res ? res.statusCode : err ? err.statusCode : 404 | |||
There was a problem hiding this comment.
While this is slick, it's not very intuitive. I rearranged it to be a little easier on the eyes.
| {visibleBookmarks.map((bookmark: MongoBookmark, index) => { | ||
| const foundBookmark = findBookmark(bookmark) | ||
| return bookmark.cmsId && foundBookmark ? ( |
There was a problem hiding this comment.
I opted to add this check for an undefined bookmark here so that we could maintain the new and improved type-safe RemovableBookmark component. So now if there is some misalignment between cmsId and id it will just display as a CustomBookmark
|
|
||
| /* MongoBookmark refers to an existing bookmark as it is stored in MongoDB. This includes | ||
| both user-created bookmarks and bookmarks that the user has added from the CMS. */ | ||
| export type MongoBookmark = { |
There was a problem hiding this comment.
I know we had briefly talked in eng sync about naming for this, MongoBookmark vs UserBookmark vs < something else >. @gidjin did you have a preference or strong opinions?
There was a problem hiding this comment.
My preference is for UserBookmark but I'm okay if it stays this way.
There was a problem hiding this comment.
I left it as MongoBookmark because, since we have bookmarks coming from two different places, it makes it more clear when working with a bookmark where it's coming from. Happy to change it if the need arises, but thought I'd clarify my intentions.
There was a problem hiding this comment.
I'm good with that reasoning. Thanks for noting it!
| url: bookmark.url, | ||
| label: bookmark.label, | ||
| collectionId, | ||
| cmsId: bookmark.id, | ||
| ...bookmark, | ||
| }, |
|
|
||
| /* MongoBookmark refers to an existing bookmark as it is stored in MongoDB. This includes | ||
| both user-created bookmarks and bookmarks that the user has added from the CMS. */ | ||
| export type MongoBookmark = { |
There was a problem hiding this comment.
My preference is for UserBookmark but I'm okay if it stays this way.
| expect(screen.getByText('3 collections selected')).toBeInTheDocument() | ||
| expect( | ||
| screen.getByText('(0 of 25 possible remaining)') | ||
| ).toBeInTheDocument() |
There was a problem hiding this comment.
Question: Did you intend to remove all the checks around the max # of collections or was that inadvertent?
There was a problem hiding this comment.
I think we should add this back, I'm going to do that today.
There was a problem hiding this comment.
I looked and this is covered just differently than before. I added a couple checks but I understand the test better now.
SC-1568
Proposed changes
/sites-and-applicationspage (forgot to do this in refactor: Refactor MySpace component #978)Still to do:
Fix testsReviewer notes
Run the app and
/sites-and-applicationsand verify that everything there works as expected (since I removed thegetMySpacequery)Please give the e2e test a look as well: https://github.com/USSF-ORBIT/ussf-portal/pull/262
Setup
yarn services:up && yarn devin CMSyarn devin portalCode review steps
As the original developer, I have
As code reviewer(s), I have
As a designer reviewer, I have
As a test user, I have
Screenshots