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

added button to duplicate roadmap #532

Merged
merged 5 commits into from
Jan 11, 2025
Merged

added button to duplicate roadmap #532

merged 5 commits into from
Jan 11, 2025

Conversation

timobraz
Copy link
Contributor

@timobraz timobraz commented Dec 7, 2024

New button to duplicate currently selected roadmap.
Will copy the content of the selected roadmap and select the copy upon duplication.

Description

Screenshots

Before:

image

After:

image

Steps to verify/test this change:

  • Verify changes work as expected on staging instance
  • Check if all the courses are transferred over
  • Check if all transfer courses are transferred over
  • Check if copy number is incremented when you have two copies
  • Check that if you save and reload all your roadmaps are there

Final Checks:

  • Verify successful deployment

(optional)

  • Write tests
  • Write documentation

Issues

Closes #519

@timobraz timobraz temporarily deployed to staging-532 December 7, 2024 03:01 — with GitHub Actions Inactive
@timobraz timobraz self-assigned this Dec 7, 2024
Copy link
Member

@Awesome-E Awesome-E left a comment

Choose a reason for hiding this comment

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

Changes seem to work correctly, and code looks good! A couple things to note though:

  • Transfers are separate from the roadmap so they're not really "copied" but rather it's just reading the same one. It does still work though
  • I think it'd make more sense to have the duplicate button between the pencil and the trash can icons, because right now it's not clear (even with text) that "duplicate roadmap" refers to the current one (it may be confused as a button to open a selection menu). I don't know how technically difficult it would be to make it so that every roadmap can be duplicated easily, but I think it's worth exploring

Copy link

@charlieweinberger charlieweinberger left a comment

Choose a reason for hiding this comment

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

LGTM. However, I agree with Ethan's 2nd point, and think if possible it should be implemented before merging with main. See AntAlmanac as an example.

@timobraz timobraz temporarily deployed to staging-532 January 6, 2025 23:42 — with GitHub Actions Inactive
@timobraz timobraz temporarily deployed to staging-532 January 6, 2025 23:43 — with GitHub Actions Inactive
@timobraz
Copy link
Contributor Author

timobraz commented Jan 6, 2025

image
Addressed the changes to look more like Ant Almanac and be more explicit about which roadmap you're duplicating.

@timobraz timobraz requested a review from Awesome-E January 7, 2025 16:18
@timobraz timobraz temporarily deployed to staging-532 January 9, 2025 16:19 — with GitHub Actions Inactive
@timobraz timobraz temporarily deployed to staging-532 January 9, 2025 16:20 — with GitHub Actions Inactive
@timobraz timobraz requested a review from Awesome-E January 9, 2025 16:21
@Awesome-E
Copy link
Member

Awesome-E commented Jan 9, 2025

Is it intended that duplicating "test (copy)" creates "test (copy) (copy)" instead of "test (copy 2)"?

because if you duplicate "test" twice it's fine, but if you duplicate the copy, it does copy copy

@timobraz
Copy link
Contributor Author

timobraz commented Jan 9, 2025

Yep as intended

@timobraz timobraz merged commit aecd5d1 into main Jan 11, 2025
2 checks passed
@timobraz timobraz deleted the tim/duplicate-roadmap branch January 11, 2025 21:07
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.

Add the ability to duplicate a roadmap
3 participants