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

#9244: add Move/Copy mod component actions #9245

Merged
merged 3 commits into from
Oct 7, 2024
Merged

#9244: add Move/Copy mod component actions #9245

merged 3 commits into from
Oct 7, 2024

Conversation

twschiller
Copy link
Contributor

@twschiller twschiller commented Oct 6, 2024 •
edited
Loading

What does this PR do?

Reviewer Notes

  • Playwright tests aren't run on CI because this is stacked on another PR. There might be bugs the Playwright will catch

Checklist

Future Work

  • In the future, consider using unsaved mod affordance instead of forcing the user to package a new mod when moving/copying to a new mod

For more information on our expectations for the PR process, see the
code review principles doc

@twschiller twschiller self-assigned this Oct 6, 2024
twschiller
twschiller commented Oct 6, 2024
View reviewed changes
src/pageEditor/modListingPanel/modals/AddToModModal.tsx Outdated
Copy link
Contributor Author

@twschiller twschiller Oct 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I decided to just drop AddToMod now. The only reason to keep it would be to handle unsaved standalone mod components that existed before #9219

After the next slice, there won't be any unsaved standalone mod components

If we don't get that slice in, users can just save the mod and then copy/move the mod component

@twschiller twschiller added page editor user experience Improve the user experience (UX) labels Oct 6, 2024
@twschiller twschiller marked this pull request as ready for review October 6, 2024 23:41
twschiller
twschiller commented Oct 6, 2024
View reviewed changes
src/pageEditor/modListingPanel/modals/CreateModModal.tsx
helpers.setSubmitting(false);
return;
}

try {
// If the active mod's saved definition could be loaded from the server, we need to use createModFromMod
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🐛 Bugfix: The order of the branches didn't work because activeModDefinition would be defined when a mod component was selected within an unsaved mod

@twschiller twschiller added this to the 2.1.4 milestone Oct 6, 2024
@twschiller twschiller mentioned this pull request Oct 7, 2024
Merged
@twschiller twschiller added the do not merge Merging of this PR is blocked by another one label Oct 7, 2024
@twschiller twschiller force-pushed the feature/9241-duplicate-component branch from 259e03b to b42c9be Compare October 7, 2024 18:03
Base automatically changed from feature/9241-duplicate-component to main October 7, 2024 18:26
@twschiller
9244: move/copy mod component
9c49084 
Drop invalid page editor page object method

Fix CreateModModalBody logic
@twschiller twschiller force-pushed the feature/9244-move-copy-mod-component branch from 99f9601 to 9c49084 Compare October 7, 2024 18:27
twschiller
twschiller commented Oct 7, 2024
View reviewed changes
end-to-end-tests/pageObjects/pageEditor/pageEditorPage.ts
@@ -238,34 +289,6 @@ export class PageEditorPage extends BasePageObject {
await deactivateModModal.deactivateButton.click();
}

@ModifiesModFormState
async createModFromModComponent({
Copy link
Contributor Author

@twschiller twschiller Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Method was not used in any tests. Uses the modal that's been dropped

@twschiller twschiller removed the do not merge Merging of this PR is blocked by another one label Oct 7, 2024
@codecov Codecov
Copy link

codecov bot commented Oct 7, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 34.72222% with 47 lines in your changes missing coverage. Please review.

Project coverage is 75.03%. Comparing base (8318d74) to head (25081de).
Report is 345 commits behind head on main.

Files with missing lines Patch % Lines
...itor/modListingPanel/modals/MoveCopyToModModal.tsx 34.09% 29 Missing ⚠️
...geEditor/modListingPanel/modals/CreateModModal.tsx 0.00% 7 Missing ⚠️
src/pageEditor/store/editor/editorSelectors.ts 16.66% 5 Missing ⚠️
src/pageEditor/store/editor/editorSlice.ts 42.85% 4 Missing ⚠️
...itor/modListingPanel/DraftModComponentListItem.tsx 60.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #9245      +/-   ##
==========================================
+ Coverage   74.24%   75.03%   +0.78%     
==========================================
  Files        1332     1371      +39     
  Lines       40817    42374    +1557     
  Branches     7634     7922     +288     
==========================================
+ Hits        30306    31795    +1489     
- Misses      10511    10579      +68

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 7, 2024 •
edited
Loading

Playwright test results

passed  134 passed
flaky  4 flaky
skipped  4 skipped

Details

report  Open report ↗︎
stats  142 tests across 46 suites
duration  12 minutes, 37 seconds
commit  25081de
info  For more information on how to debug and view this report, see our readme

Flaky tests

chrome › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod
msedge › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod
chrome › tests/pageEditor/copyMod.spec.ts › copying a mod that uses the PixieBrix API is copied correctly
msedge › tests/regressions/welcomeStarterBricks.spec.ts › #8740: can view the starter mods on the pixiebrix.com/welcome page

Skipped tests

chrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
msedge › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor
chrome › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options
msedge › tests/runtime/googleSheetsIntegration.spec.ts › can activate a google spreadsheet mod with config options

mnholtz
mnholtz reviewed Oct 7, 2024
View reviewed changes
end-to-end-tests/tests/pageEditor/moveCopyModComponent.spec.ts
Copy link
Collaborator

Choose a reason for hiding this comment

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

We might want to consider adding more test coverage per moving/copying mod components to existing mods

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 7, 2024

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

@twschiller twschiller enabled auto-merge (squash) October 7, 2024 21:04
@twschiller twschiller merged commit f75a3d1 into main Oct 7, 2024
20 checks passed
@twschiller twschiller deleted the feature/9244-move-copy-mod-component branch October 7, 2024 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnholtz mnholtz mnholtz approved these changes

@grahamlangford grahamlangford Awaiting requested review from grahamlangford

@fungairino fungairino Awaiting requested review from fungairino

Assignees

@twschiller twschiller

Labels
page editor user experience Improve the user experience (UX)
Projects
None yet
Milestone
2.1.5
Development

Successfully merging this pull request may close these issues.

Slice 5: Convert “Move from mod” action to “Move to Mod” and adjust “Copy to Mod” action
2 participants
@twschiller @mnholtz