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

SEAB-5183: created notebooks page and added to sitemap #1692

Merged
merged 10 commits into from
Feb 2, 2023

Conversation

hyunnaye
Copy link
Contributor

@hyunnaye hyunnaye commented Jan 25, 2023

Description
Created notebooks page dockstore.org/notebooks and added the link to the sitemap.

Notebooks page
Screenshot from 2023-01-31 16-36-09

Sitemap
Screenshot from 2023-01-31 12-16-22

Review Instructions
Verify the new page renders and is implemented correctly and can be accessed from the sitemap.

Issue
https://ucsc-cgl.atlassian.net/browse/SEAB-5183

Security
If there are any concerns that require extra attention from the security team, highlight them here.

Please make sure that you've checked the following before submitting your pull request. Thanks!

  • Check that your code compiles by running npm run build
  • Ensure that the PR targets the correct branch. Check the milestone or fix version of the ticket.
  • If this is the first time you're submitting a PR or even if you just need a refresher, consider reviewing our style guide
  • Do not bypass Angular sanitization (bypassSecurityTrustHtml, etc.), or justify why you need to do so
  • If displaying markdown, use the markdown-wrapper component, which does extra sanitization
  • Do not use cookies, although this may change in the future
  • Run npm audit and ensure you are not introducing new vulnerabilities
  • Do due diligence on new 3rd party libraries, checking for CVEs
  • Don't allow user-uploaded images to be served from the Dockstore domain
  • If this PR is for a user-facing feature, create and link a documentation ticket for this feature (usually in the same milestone as the linked issue). Style points if you create a documentation PR directly and link that instead.
  • Check whether this PR disables tests. If it legitimately needs to disable a test, create a new ticket to re-enable it in a specific milestone.

@hyunnaye hyunnaye self-assigned this Jan 25, 2023
@hyunnaye hyunnaye changed the title SEAB-5103: created notebooks page and added to sitemap SEAB-5183: created notebooks page and added to sitemap Jan 25, 2023
@hyunnaye
Copy link
Contributor Author

Will need to wait for dockstore/dockstore#5327 to be merged for the tests to pass

@codecov
Copy link

codecov bot commented Jan 31, 2023

Codecov Report

Base: 40.77% // Head: 40.78% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (9a64077) compared to base (0a73edc).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1692   +/-   ##
========================================
  Coverage    40.77%   40.78%           
========================================
  Files          348      348           
  Lines        10612    10613    +1     
  Branches      2719     2719           
========================================
+ Hits          4327     4328    +1     
  Misses        4053     4053           
  Partials      2232     2232           
Impacted Files Coverage Δ
...p/workflows/list/published-workflows.datasource.ts 39.28% <ø> (ø)
src/app/shared/enum/entry-type.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@coverbeck coverbeck left a comment

Choose a reason for hiding this comment

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

One part optional, but I have the question about the image.

@@ -17,7 +17,12 @@
<div fxLayout="row" fxLayoutAlign="start center">
<img src="../assets/svg/sub-nav/{{ (entryPageTitle$ | async | titlecase) === 'Services' ? 'services.svg' : 'workflow.svg' }}" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be updated? If not, and you're planning to share the same image for notebooks, can you please add a comment or an explicit if condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it needs to be updated. I'll look for some icons that can be used for notebooks.

Copy link
Contributor Author

@hyunnaye hyunnaye Jan 31, 2023

Choose a reason for hiding this comment

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

Added a placeholder image (https://fontawesome.com/icons/book?s=solid&f=classic) and set the color to #3f51b5 which is the first Accent 1 colour in zeplin palette https://app.zeplin.io/project/5fc690a2717ee9144814f190/screen/5fc699d068c46f17551f745b

Screenshot of this change is added onto PR description

Copy link
Contributor

Choose a reason for hiding this comment

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

Added a placeholder image (https://fontawesome.com/icons/book?s=solid&f=classic) and set the color to #3f51b5 which is the first Accent 1 colour in zeplin palette https://app.zeplin.io/project/5fc690a2717ee9144814f190/screen/5fc699d068c46f17551f745b

By placeholder, do you mean temporary? If yes, can you please create a followup ticket.

If no, would it be possible to use the fontawesome image that we already bundle, like here. That way we don't have to bundle another copy, and it stays in sync/style with the other fontawesome icons if we upgrade the library. The downside is that you would use different HTML for this one case, and maybe we don't want the icon to stay in sync with the other fontawesome icons, we want it to stay in sync with the workflows/services SVGs, so I'm not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created ticket dockstore/dockstore#5343

src/app/workflows/workflows.component.html Outdated Show resolved Hide resolved
Copy link
Member

@denis-yuen denis-yuen left a comment

Choose a reason for hiding this comment

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

check the checklist, probably didn't run into anything but worth double checking

@sonarcloud
Copy link

sonarcloud bot commented Feb 2, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
6.2% 6.2% Duplication

@hyunnaye hyunnaye merged commit 853780a into develop Feb 2, 2023
@hyunnaye hyunnaye deleted the feature/SEAB-5103/notebooks-page branch February 2, 2023 19:08
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.

5 participants