search

allenai / pdf-component-library

51 stars 5 forks source link

Adding initial, hover, active state for bounding box in Pdf Component Library #116

Closed huytr1995 closed 2 years ago

huytr1995 commented 2 years ago

Description

Ref: https://github.com/allenai/scholar/issues/31196 Figma: https://www.figma.com/file/2gUmD3D8tz6gnm9rsYgj6J/Semantic-Reader?node-id=350%3A9087

Reviewer Instruction

This is the first step of adding the css change for bounding box, there will be a separate PR to increase the package version and publish it and then will be another one to do the same in S2. There are 3 new state for the BoundingBox:

  1. Initial: #1857B6
  2. Hover: #5492EF
  3. Active: #5492EF Each state will include the underline dot per discussion with Cassidy

Testing plan

  1. Manual testing to verify the demo app to verify it has all the state as Cassidy request.
  2. Yarn lint, format, test

Output

https://user-images.githubusercontent.com/84343285/161606646-f1eb6d9e-28b4-41e0-bb64-fe2444656c7e.mov

carolinepaulic commented 2 years ago

@huytr1995 Have you thought about putting these changes in S2 instead of in the library? The styles you added are specific to our app.

carolinepaulic commented 2 years ago

@huytr1995 I pulled your branch but I don't see the bottom border on the bounding boxes. Is that still working on your end? image

image

carolinepaulic commented 2 years ago

The demo citation popovers are also acting weird with their placement... possibly related to the border issue? 🤔

huytr1995 commented 2 years ago

The demo citation popovers are also acting weird with their placement... possibly related to the border issue? 🤔

@carolinepaulic if u check out main branch and run the app the pop over issue has been like this without my changes

huytr1995 commented 2 years ago

@huytr1995 I pulled your branch but I don't see the bottom border on the bounding boxes. Is that still working on your end? image

image

@carolinepaulic it still does, have u run yarn build in the ui/library??

carolinepaulic commented 2 years ago
carolinepaulic commented 2 years ago

@huytr1995 I pulled your branch but I don't see the bottom border on the bounding boxes. Is that still working on your end? @carolinepaulic it still does, have u run yarn build in the ui/library??

@huytr1995 Yep doing a build fixed it the bottom border, thanks. Our standard in the past has been to keep the built files in sync for the demo.

It is missing the cursor: pointer; styling on hover though.

carolinepaulic commented 2 years ago

The bottom border doesn't respect page rotation. You should be testing all of the existing library features before pushing up changes and you should be describing the manual testing that you performed in detail in your PR description.

huytr1995 commented 2 years ago
  • Did you intend to add ShareButton.tsx?
  • Why did you remove pdf-components.css and index.d.ts?
  • Don't forget to combine the hover and selected states in styles.less

@carolinepaulic 1/ That would be in separate PR. I created a ticket for it already. 2/ As Yensung suggested last time, adding those files from dist folder will be a lot and hard to read so better do it in a PR with package.json upgrade 3/ I will do it now

carolinepaulic commented 2 years ago
  • Did you intend to add ShareButton.tsx?
  • Why did you remove pdf-components.css and index.d.ts?
  • Don't forget to combine the hover and selected states in styles.less

@carolinepaulic 1/ That would be in separate PR. I created a ticket for it already. 2/ As Yensung suggested last time, adding those files from dist folder will be a lot and hard to read so better do it in a PR with package.json upgrade 3/ I will do it now

@huytr1995

  1. Ok, you should remove that file from this PR then. Looks like it got mixed in on accident.
  2. Gotcha, you should probably just remove your changes to those files instead of deleting the entire file.
  3. Looks good, thanks!