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

Feature/conversion title #559

Merged
merged 4 commits into from
Sep 30, 2024
Merged

Feature/conversion title #559

merged 4 commits into from
Sep 30, 2024

Conversation

interim17
Copy link
Contributor

@interim17 interim17 commented Sep 16, 2024

Time estimate or Size

small
Will get review at UX office hours prior to merging, code ready for review.

Problem

Advances #396
Closes: #509

Solution

UI for input, and optional property on ConversionProcessingData for title.

Small tweaks to the naming of fileName and backendFileName for clarity in the file conversion logic.

Steps to Verify:

  1. Send a Smoldyn file for conversion and provide a title, check that the title appears in the trajectory you receive back.
    (Tip: logging the data on line 271 in ViewerPanel.tsx in handleIncomingConvertedFile)

Screenshots (optional):

Screenshot 2024-09-16 at 9 20 06 AM

Copy link

github-actions bot commented Sep 16, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
66.4% (-0.27% 🔻)
666/1003
🟡 Branches
65.31% (-0.9% 🔻)
96/147
🔴 Functions
35.41% (-0.42% 🔻)
91/257
🟡 Lines
64.7% (-0.21% 🔻)
592/915
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🔴
... / reducer.ts
20% (-1.43% 🔻)
100% 0%
20% (-1.43% 🔻)
🔴
... / actions.ts
51.28% (-0.07% 🔻)
100% 0%
51.28% (-0.07% 🔻)
🔴
... / logics.ts
15.35% (-0.14% 🔻)
0% 0%
15.77% (-0.07% 🔻)

Test suite run success

121 tests passing in 7 suites.

Report generated by 🧪jest coverage report action from 3fa65a0

@interim17 interim17 marked this pull request as ready for review September 16, 2024 16:33
@interim17 interim17 requested a review from a team as a code owner September 16, 2024 16:33
@interim17 interim17 requested review from toloudis and tyler-foster and removed request for a team September 16, 2024 16:33
Copy link
Contributor

@toloudis toloudis left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@tyler-foster tyler-foster left a comment

Choose a reason for hiding this comment

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

LGTM.

Do we care about the overall test coverage going down? Should there be tests for e.g. the reducer.ts and logics.ts changes?

@interim17
Copy link
Contributor Author

LGTM.

Do we care about the overall test coverage going down? Should there be tests for e.g. the reducer.ts and logics.ts changes?

Talked to Dan about this and at least for now he seems to feel comfortable with overall coverage dropping below the previous threshold.

@interim17 interim17 merged commit 8c1e036 into main Sep 30, 2024
6 checks passed
@interim17 interim17 deleted the feature/conversion-title branch September 30, 2024 21:32
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.

[auto-conversion] - allow optional user submitted title for converted trajectory
3 participants