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

Add configurable about this dataset links #907

Merged
merged 16 commits into from Sep 26, 2019
Merged

Conversation

seve
Copy link
Member

@seve seve commented Sep 3, 2019

This PR introduces configurable link outs to additional information about the dataset. These links are featured conditionally in place of the dataset name in the top-left corner and in the info menu in the top-right.
image
image


This PR implements the following changes:

  • Server
    • cli/launch.py
      • Add --about flag and description
      • Add link validator and raise an exception if fails
      • Pass the link to config
    • app/rest_api/rest.py
      • add links field to config JSON
  • Client
    • globals
      • add empty default for links
    • informationMenu component
    • Add a conditionally rendered menu item
    • menubar component
      • pass about link as prop to info menu
    • TopLeftLogoAndTitle component
      • Conditionally turn the dataset name into a link

This PR will close #902

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.

This looks great to me! Thanks for getting this in so quickly! One tiny thing I'd change is the info button link from Title Case to just Sentence case (make "this" and "dataset" lowercase, but keep "About" capitalized).

@seve
Copy link
Member Author

seve commented Sep 3, 2019

Should note that I haven't added any styles and haven't put much thought into the icon for the menu item. I'm open to any suggestion for the two.

@seve
Copy link
Member Author

seve commented Sep 3, 2019

@bkmartinjr Could you review the server-side of this PR? This is my first time touching that codebase.

server/cli/launch.py Outdated Show resolved Hide resolved
server/cli/launch.py Outdated Show resolved Hide resolved
server/cli/launch.py Outdated Show resolved Hide resolved
@bkmartinjr
Copy link
Contributor

@seve - please fix the build fail (due to lint added to the Python code). See the Travis error output: server/cli/launch.py:98:1: E304 blank lines found after function decorator.

You can run this on your own box by running the same commands Travis runs. See the .travis.yml file. In this case, I believe flake8 is erroring.

Copy link
Contributor

@bkmartinjr bkmartinjr left a comment

Choose a reason for hiding this comment

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

Several items I recommend changing:

  • fix the lint (build failure)
  • change "link" to "url", as that is much more accurate
  • (optional) improve the URL validation to check for HTTP scheme
  • remove the unused flask template variable

See inline comments.

@@ -41,7 +42,8 @@ class LeftSideBar extends React.Component {
userSelect: "none"
}}
>
cell<span
cell
<span
style={{
position: "relative",
top: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

The alignment of this dataset title looks correct, baseline aligned with cellxgene

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this compared to the issue we were seeing in #909?

Copy link
Member Author

Choose a reason for hiding this comment

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

You marked requesting changed, but not sure what needs changing

client/src/components/menubar/infoMenu.js Outdated Show resolved Hide resolved
@bkmartinjr
Copy link
Contributor

@seve - you will need to resolve the merge conflict before this PR can be approved.

server/cli/launch.py Outdated Show resolved Hide resolved
@seve seve merged commit 44cb276 into master Sep 26, 2019
@seve seve deleted the seve/feature-about-link branch October 2, 2019 19:39
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.

Configurable 'more information' link outs
4 participants