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

Within libraries, open editors in a modal, not a new URL #1321

Closed
Tracked by #1086
bradenmacdonald opened this issue Sep 24, 2024 · 17 comments
Closed
Tracked by #1086

Within libraries, open editors in a modal, not a new URL #1321

bradenmacdonald opened this issue Sep 24, 2024 · 17 comments
Labels
enhancement Relates to new features or improvements to existing features

Comments

@bradenmacdonald
Copy link
Contributor

bradenmacdonald commented Sep 24, 2024

As an author, when I close out of an editor modal I expect to be right back where I just was, not taken to the library homepage.

As an author, I want to see at least a little bit of context when I'm editing a component. Example:

image

(Note how the course, section, and subsection are visible)

@kdmccormick
Copy link
Member

@bradenmacdonald
Copy link
Contributor Author

@kdmccormick I'll see what I can do when I work on this, but because we're still using the legacy Unit page, it's likely that this change is only going to be an improvement for people using these editors from a content library, and not yet from a course.

@kdmccormick
Copy link
Member

I think that's reasonable. Plus, it gives people a reason to look forward to the new course unit editor.

@bradenmacdonald
Copy link
Contributor Author

@jmakowski1123 @sdaitzman @lizc577 This is ready for AC testing on the sandbox.

@bradenmacdonald bradenmacdonald moved this from In Progress to Ready for AC testing in Libraries Overhaul Oct 8, 2024
@jmakowski1123
Copy link

Looking great. This is a super tiny nit, and in no way a blocker. When I click on any component, there's a super brief spinner that pops up before the modal. It's performing so quickly on my instance that it's so quick, you can barely process what you're seeing, but it's a little jarring. We definitely need a spinner or some indicator of loading if there's a delay, but when it opens so quickly, it's a bit jarring. Is there any solution for that? Some microsecond threshold where no spinner displays? Again, tiny nit.

@bradenmacdonald
Copy link
Contributor Author

@jmakowski1123 I'm sure there's something we can do but it's not a trivial fix. Let's take a look after MVP ? We can always fix it for Sumac as a bugfix.

@jmakowski1123
Copy link

Totally fine

@sdaitzman
Copy link

Looks good to me, the modal is great. Two small UX nits:

  1. When opening a problem or text component for editing, then closing it without making any edits, I notice that there is still a confirmation prompt. If no changes have been made, is it possible to allow the user to exit the modal without that confirmation?
  2. Reloading or otherwise using browser controls to navigate to another page loses unsaved changes with no confirmation. Can we intercept that event and warn the user they may lose unsaved changes in that case?

(There are some potential further UX improvements to this, like caching edits across reloads, but I think those are well post-MVP scope)

@bradenmacdonald
Copy link
Contributor Author

@sdaitzman Those are excellent points. Neither of those issues are new - they've been around since the MFE editors were introduced (in courses). I can definitely fix both of them, but I'm not sure how to prioritize them relative to our other work on the MVP.

@sdaitzman
Copy link

@bradenmacdonald I'll defer to @jmakowski1123 on the level of priority/whether fixing these MFE issues is in-scope as part of this work.

The unnecessary confirmation would appear more frequently for users regularly opening components for editing, but is basically just a small annoyance. Most users can expand the component preview as a workaround for MVP.

The browser navigation issue would probably not occur very often, but would have a larger impact each time, since users could lose their recent edits.

@pomegranited
Copy link
Contributor

@bradenmacdonald I'm seeing a strange thing now when I paste Problem components into a Content Library: no matter what their problem type is, when I Edit the new pasted library problem, it always shows the OLX in the Advanced problem editor instead of using the nice problem-type-specific editor. I've reproduced this on tagging-preview too (see e.g. "Multiple Choice" problem in lib:OpenCraft:TL).

Is this already a known issue? I can address next sprint as part of FAL-3873.

@bradenmacdonald
Copy link
Contributor Author

@pomegranited No, I wasn't aware of that issue. It sounds similar to #1326 though. If you can fix it as part of FAL-3873, that would be great!

@jmakowski1123
Copy link

jmakowski1123 commented Oct 16, 2024

@bradenmacdonald I'm seeing a strange thing now when I paste Problem components into a Content Library: no matter what their problem type is, when I Edit the new pasted library problem, it always shows the OLX in the Advanced problem editor instead of using the nice problem-type-specific editor. I've reproduced this on tagging-preview too (see e.g. "Multiple Choice" problem in lib:OpenCraft:TL).

Is this already a known issue? I can address next sprint as part of FAL-3873.

I noted this was happening here (#1091 (comment)) - it seems to have been connected to the max attempts setting, but I couldn't say for sure that was the pattern.

@jmakowski1123
Copy link

@bradenmacdonald I'll defer to @jmakowski1123 on the level of priority/whether fixing these MFE issues is in-scope as part of this work.

The unnecessary confirmation would appear more frequently for users regularly opening components for editing, but is basically just a small annoyance. Most users can expand the component preview as a workaround for MVP.

The browser navigation issue would probably not occur very often, but would have a larger impact each time, since users could lose their recent edits.

I wouldn't prioritize either of these in front of Epic 6 for the MVP. I think it could be argued that both of these could be classified as bugs. Just created two bug tickets and we can prioritize them against all the other library bugs during the 6 week testing period. Agree the navigation issue carries more weight.

@bradenmacdonald
Copy link
Contributor Author

@jmakowski1123 With those two changes now made into separate tickets, can we mark this as done?

@jmakowski1123
Copy link

I was waiting until the legacy editor reversion issue was resolved, which I realize is being tracked separately, but is tied to tests on this ticket. Is that bug fixed?

@jmakowski1123
Copy link

#1377 Tracking this bug separately, so closing this issue.

@jmakowski1123 jmakowski1123 moved this from Ready for AC testing to Done in Libraries Overhaul Oct 24, 2024
@jmakowski1123 jmakowski1123 closed this as completed by moving to Done in Libraries Overhaul Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Relates to new features or improvements to existing features
Projects
Status: Done
Development

No branches or pull requests

5 participants