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

[ENG-5012][ENG-5014] Updates to see the file version for the latest preprint deploy #2075

Conversation

bp-cos
Copy link
Contributor

@bp-cos bp-cos commented Dec 5, 2023

Purpose

The file name, version and ability to download a different version was not included with the latest release

Summary of Changes

Ported the "file renderer" component from OSF Preprints

Screenshot(s)

Screenshot 2023-12-05 at 4 12 55 PM

Side Effects

N/A

QA Notes

This is challenging to test locally because the MFR does not work.

@bp-cos bp-cos requested review from adlius and futa-ikeda December 5, 2023 23:13
@coveralls
Copy link

coveralls commented Dec 5, 2023

Pull Request Test Coverage Report for Build 7134599248

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 0 of 22 (0.0%) changed or added relevant lines in 2 files are covered.
  • 36 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.2%) to 69.053%

Changes Missing Coverage Covered Lines Changed/Added Lines %
app/preprints/detail/route.ts 0 1 0.0%
app/preprints/-components/preprint-file-render/component.ts 0 21 0.0%
Files with Coverage Reduction New Missed Lines %
lib/osf-components/addon/components/search-page/component.ts 10 77.24%
lib/osf-components/addon/components/search-result-card/component.ts 10 21.21%
app/preprints/detail/route.ts 16 0.0%
Totals Coverage Status
Change from base Build 7089267861: -0.2%
Covered Lines: 6068
Relevant Lines: 8537

💛 - Coveralls

Copy link
Contributor

@futa-ikeda futa-ikeda left a comment

Choose a reason for hiding this comment

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

I've got a couple of possible cleanup areas, and it doesn't looks like allowCommenting is doing anything right now.

mirage/factories/file-version.ts Outdated Show resolved Hide resolved
mirage/factories/file.ts Outdated Show resolved Hide resolved
mirage/factories/preprint.ts Outdated Show resolved Hide resolved
<div local-class='file-renderer-container {{if this.isMobile 'mobile'}}'>
<div local-class='file-container'>
<FileRenderer
@allowCommenting={{this.allowCommenting}}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like FileRenderer isn't doing anything with this new parameter here. It looks like it was used to determine if we needed to do a postMessage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was planning on tackling this after the hot fix since it's not part of the issue right now. I can fix it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can address it in a separate JIRA ticket/PR if it doesn't seem easy to implement. I'll leave that call up to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the allowCommenting parameter is being used to enable Hypothesis though. I'm not the most familiar with how Hypothesis works, but in ember-osf, there is some window.postMessage being done based on allowCommenting. I'll reach out to others who may be more familiar with how this works to see what needs to be done

app/models/file-version.ts Show resolved Hide resolved
@bp-cos bp-cos changed the title [DRAFT] [ENG-5012] Updates to see the file version for the latest preprint deploy [ENG-5012] Updates to see the file version for the latest preprint deploy Dec 6, 2023
@futa-ikeda futa-ikeda changed the title [ENG-5012] Updates to see the file version for the latest preprint deploy [ENG-5012][ENG-5014] Updates to see the file version for the latest preprint deploy Dec 7, 2023
Copy link
Contributor

@futa-ikeda futa-ikeda left a comment

Choose a reason for hiding this comment

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

🔥 🦊

@adlius adlius merged commit 3255af4 into CenterForOpenScience:hotfix/23.15.2 Dec 8, 2023
9 checks passed
adlius pushed a commit that referenced this pull request Dec 12, 2023
…reprint deploy (#2075)

* Updates to see the file version

* Added mobile formatting, a trait for different images

* Fixed the sorting and versioning on the file versions

* Updates per the PR

* Updated the file-renderer to use "allowCommenting". Updated the tests

* Revert "Updated the file-renderer to use "allowCommenting". Updated the tests"

This reverts commit 3b274d8.

* Fixed all the issues from the PR

* Fixed the date created issue

* A few minor fixes
bp-cos added a commit to bp-cos/ember-osf-web that referenced this pull request Jan 31, 2024
…reprint deploy (CenterForOpenScience#2075)

* Updates to see the file version

* Added mobile formatting, a trait for different images

* Fixed the sorting and versioning on the file versions

* Updates per the PR

* Updated the file-renderer to use "allowCommenting". Updated the tests

* Revert "Updated the file-renderer to use "allowCommenting". Updated the tests"

This reverts commit 3b274d8.

* Fixed all the issues from the PR

* Fixed the date created issue

* A few minor fixes
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.

4 participants