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.

Sign up for GitHub

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

Jump to bottom

Preview page metadata highlighting #291

Merged
merged 19 commits into from
Oct 25, 2021
Merged

Preview page metadata highlighting #291

merged 19 commits into from
Oct 25, 2021

Conversation

klai95
Copy link
Contributor

@klai95 klai95 commented Oct 21, 2021 •
edited
Loading

@klai95 klai95 requested a review from codemonkey800 October 21, 2021 00:09
codemonkey800
codemonkey800 requested changes Oct 21, 2021
View reviewed changes
Copy link
Collaborator

@codemonkey800 codemonkey800 left a comment

Choose a reason for hiding this comment

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

Looking good overall! I posted a few comments with some minor changes, but the main thing right now is the status icons aren't being rendered correctly (possibly because of a flex layout somewhere)

Actual

image

Expected

Figma

image

frontend/src/components/PluginDetails/MetadataList.tsx Outdated Show resolved Hide resolved
frontend/src/fixtures/napari.json Outdated Show resolved Hide resolved
@codemonkey800
Copy link
Collaborator

Also, the orange background should be behind the title if it's "information not submitted" 🤔

@codemonkey800
Copy link
Collaborator

@klai95 I also tagged Lia for design review. When you get a chance after fixing the above issues, could you create a dev-* branch and share the URL so that we can test out the live feature pls? 😄

@klai95
Copy link
Contributor Author

klai95 commented Oct 21, 2021

@klai95 I also tagged Lia for design review. When you get a chance after fixing the above issues, could you create a dev-* branch and share the URL so that we can test out the live feature pls? 😄

@codemonkey800 of course!

@klai95 klai95 force-pushed the preview-page-metadata-highlight branch from 91c1167 to 1f09c0e Compare October 22, 2021 22:53
@liaprins-czi
Copy link

Is there a link where I can see the latest? It looks like there have been some updating since the last screenshots... or else a screenshot of the latest would be just as good for my needs! Thanks! @klai95 @codemonkey800

@klai95
Copy link
Contributor Author

klai95 commented Oct 25, 2021

Is there a link where I can see the latest? It looks like there have been some updating since the last screenshots... or else a screenshot of the latest would be just as good for my needs! Thanks! @klai95 @codemonkey800

The PR screenshot that I have included at the top of this webpage is the up to date screenshot! The recent commits have been for refactoring!

@codemonkey800
Copy link
Collaborator

Is there a link where I can see the latest? It looks like there have been some updating since the last screenshots... or else a screenshot of the latest would be just as good for my needs! Thanks! @klai95 @codemonkey800

@liaprins-czi we updated the scope of this PR to only be the functionality, so that only includes the highlighting and tooltip. a future PR will focus on the styling, mobile breakpoints, and scrolling 👍

liaprins-czi
liaprins-czi approved these changes Oct 25, 2021
View reviewed changes
Copy link

@liaprins-czi liaprins-czi left a comment

Choose a reason for hiding this comment

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

Since this PR is just about the functionality of getting the flags in, and not the precise styling yet, this seems like it's working as expected from my POV!

codemonkey800
codemonkey800 approved these changes Oct 25, 2021
View reviewed changes
Copy link
Collaborator

@codemonkey800 codemonkey800 left a comment

Choose a reason for hiding this comment

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

LGTM! 📄

@klai95 klai95 merged commit f6f4cad into main Oct 25, 2021
@klai95 klai95 deleted the preview-page-metadata-highlight branch October 25, 2021 23:30
kne42 added a commit that referenced this pull request Oct 26, 2021
@kne42
Merge branch 'main' into easy-citation
d93907f 
* main: (43 commits)
  use html mime for index.html
  update s3 cache location (#304)
  Preview page base path (#302)
  Preview page metadata highlighting (#291)
  Update nvmrc to use Node.js v16 (#303)
  only set citation value when valid citation presents (#301)
  Dev GitHub preview app (#294)
  napari hub bug fixes (#290)
  Increase z-index for sticky preview bar (#293)
  Bump browserslist from 4.16.3 to 4.16.6 in /frontend (#292)
  Updates frontend dependencies (#288)
  Remove dates from preview page (#289)
  Update absolute imports for preview package
  fix module importing with absolute import
  fix module importing with absolute import
  update docker command
  Preview page app bar (#276)
  addressed provisioned lambda count default
  Fix get_preview import
  just move dockerfile out
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codemonkey800 codemonkey800 codemonkey800 approved these changes

@liaprins-czi liaprins-czi liaprins-czi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@klai95 @codemonkey800 @liaprins-czi