-
Notifications
You must be signed in to change notification settings - Fork 149
style: Fixing nits about sync units [FC-0097] #2319
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
base: master
Are you sure you want to change the base?
style: Fixing nits about sync units [FC-0097] #2319
Conversation
* Stay visible the sync icon in the course outline * Update the message in the sync unit modal * Add warning banner about units in the libraries sync page
Thanks for the pull request, @ChrisChV! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Hi @bradenmacdonald could you review this PR and the backport #2320? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2319 +/- ##
=======================================
Coverage 94.47% 94.47%
=======================================
Files 1169 1169
Lines 25134 25138 +4
Branches 5367 5369 +2
=======================================
+ Hits 23745 23749 +4
Misses 1324 1324
Partials 65 65 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@ChrisChV CC @sdaitzman @edschema Thanks! The code looks good but I notice some issues:
Screen.Recording.2025-07-23.at.10.22.39.AM.movThis may be out of scope, but when I click the sync button, this dialog is very confusing: "The old version preview is the previous library version" (???). First of all, just parsing this sentence is hard. Is it saying "The old 'version preview' is the previous 'library version'?" or "The 'old version' preview is the 'previous' library version"? I can figure out what it means, but it's definitely confusing, and I think it's presented with way too much emphasis for such a subtle distinction. Plus, there is no actual preview at all (for units), so why do we care? ![]() ✅ This part looks good: ![]() |
@bradenmacdonald thanks for the detailed look and review. I agree, the hover behavior should be consistent between library and locally created containers:
I'm okay with the draft chip alignment shifting with the sync icon. Is that okay with you @sdaitzman? On Unit sync: Agreed on the messaging and (non) utility with no available previews, but I want to hold off on making any suggestions until we have a bit more clarity on the scope sync in Ulmo..! |
This happens because the "Edit" button is disabled for library-linked units. In this case, I think it would be better for the user to hide that button. Since adding the hover circle state, it would appear to be activated, but when the user clicks it, nothing happens; at first glance, it would seem like a bug. What do you think? |
@ChrisChV I see. It's really not obvious to me that it's disabled. I think it needs to be more grey / faded out, or hidden altogether. |
I agree. @edschema what do you think? Are you ok with hiding the "Edit" button? |
I think it's fine in this case to hide the "Edit" button as long as the objects include a library indicator (which I think is the case as of #2167). In a lot of cases, it's good UX practice to leave core elements like this visible and mark them as non-unavailable/inactive (in this case probably using use a lighter gray in addition to removing the highlight circle). I think it's alright in this case as long as the library unit includes a library icon. That provides some context for why the edit button is missing. Some design guidelines for reference: |
@bradenmacdonald It's ready for another review |
Hi @sdaitzman, I used the ![]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrisChV looks good! Thanks.
Hi @ChrisChV, a similar style to this would makes sense to me, but it looks a bit lighter than I would expect. I wanted to confirm whether there's a standard Paragon style for this, or other platform examples we can match. @edschema found a similar example here under the group configuration: https://app.modular-learning-preview.opencraft.hosting/authoring/course/course-v1:eddieX+ed101+1/group_configurations Would it be possible to match this style, with a tooltip text like "This object was added from a library, so it cannot be edited" or similar? Screen.Recording.2025-08-08.at.4.39.15.PM.movApologies for the back-and-forth on this UI issue, I've been trying to track down a clear Paragon/styleguide recommendation for this case and it's a bit inconsistent within the platform. I may look into proposing a consistent style for this case to simplify this. |
@bradenmacdonald This last change is ready for a review 914cc23 @sdaitzman This is the result. Thanks for the example! ![]() |
@ChrisChV As mentioned on Slack, if this is the outline page, I think that button is just for renaming so we do want to allow renaming units even if they're from a library (right?). But on the unit page, we don't want to allow edits to components that come from a library, and their edit button should look like that. |
@bradenmacdonald CC @edschema I see, you are right, but in Teak, we should leave this button disabled for units in the course outline, right? Another question: In the Teak branch, I've tested that editing library components in the unit outline (not a unit from a library) is enabled. Is that correct? |
Description
Supporting information
Testing instructions
Content > Library
updated, and verify the new warning about unit updatesOther information
Best Practices Checklist
We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:
.ts
,.tsx
).propTypes
,defaultProps
, andinjectIntl
patterns are not used in any new or modified code.src/testUtils.tsx
(specificallyinitializeMocks
)apiHooks.ts
in this repo for examples.messages.ts
files have adescription
for translators to use.../
. To import from parent folders, use@src
, e.g.import { initializeMocks } from '@src/testUtils';
instead offrom '../../../../testUtils'