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 Request]: Improve the experience of adding concept cards in the exploration editor. #18508

Closed
4 tasks done
seanlip opened this issue Jun 26, 2023 · 9 comments
Closed
4 tasks done
Assignees
Labels
enhancement Label to indicate an issue is a feature/improvement Impact: High Blocks or significantly slows down a core workflow. Work: High It's not clear what the solution is; will need investigation.

Comments

@seanlip
Copy link
Member

seanlip commented Jun 26, 2023

Is your feature request related to a problem? Please describe.

The experience of adding skill concept cards in the exploration editor has a number of issues:

  • The highlighted text in the RTE does not get transferred to the concept card skill link modal when it is opened.
  • Cancelling the operation results in the disappearance of the highlighted text.
  • The list of skills takes a long time to load, even if it has already been fetched previously.
  • Terms entered in the skill filter input box do not get applied when the skill list finally loads.
  • The detailed skill explanations show up in red, which is hard to read (and suggests some kind of error).

See the video below for the current experience:

bug-report-A2-2023-06-26_16.43.07.mp4

Describe the solution you'd like

  • When the skill link modal is opened, the text input field should be populated with the highlighted text in the RTE, or "concept card" if there is no highlighted text.
  • When the modal is cancelled, the RTE contents should stay as they were before the modal was opened. In particular, any highlighted text should not be deleted.
  • The skill filter input box should be disabled until the list of skills has loaded.
  • Skill explanations should show up in black text.

Describe alternatives you've considered

  • I considered also writing "the list of skills should load faster", but in reality there's always going to be some lag time. It might be possible to cache this list so that it loads fast for the second and subsequent queries, but that could lead to problems with stale data if a skill is created/deleted while an exploration is being edited. So I suggest that we don't solve that problem in this issue, and fix the other points first -- then, we can see if the loading time is still an issue.

Additional context

No response

@seanlip seanlip added triage needed enhancement Label to indicate an issue is a feature/improvement and removed triage needed labels Jun 26, 2023
@prafulbbandre prafulbbandre added Impact: High Blocks or significantly slows down a core workflow. Work: High It's not clear what the solution is; will need investigation. labels Aug 14, 2023
@jnvtnguyen jnvtnguyen self-assigned this Jan 23, 2024
@jnvtnguyen
Copy link
Contributor

@seanlip Just to confirm a few things

  1. When canceling the highlighted portion should not be a RTE component anymore and also it shouldn't be deleted?
  2. When highlighting and creating a concept card, the delete button shouldn't be there and only the cancel button?

@seanlip
Copy link
Member Author

seanlip commented Jan 24, 2024

  1. Yes, correct.
  2. Yes, I think that would be fine, since one can already just delete directly from the RTE itself. But I have less strong feelings about this as long as it's clear to the user what is going on and the behaviour for all flows is conceptually correct. (I'm a bit unsure if I've interpreted your question correctly, though, since I don't see a delete button in the modal currently?)

Thanks for checking!

@jnvtnguyen
Copy link
Contributor

One more thing I believe that would improve user experience is making the skill id required because right now the user can save a concept card without a skill and when saving it will emit an error. This should be handled beforehand by disabling the submit button when not selected.

@seanlip
Copy link
Member Author

seanlip commented Jan 24, 2024

@jnvtnguyen I think the skill ID is actually assigned by the system, not the user. Can you show a video demonstrating how the user is able to save a skill when the ID isn't provided, please? (Not sure if we are thinking about the same thing here.)

Thanks!

@jnvtnguyen
Copy link
Contributor

@seanlip
Copy link
Member Author

seanlip commented Jan 24, 2024

Ah got it -- thanks @jnvtnguyen. Yes I agree, would be good to handle this.

As an FYI there is a similar issue open in #18211 for videos, and the PR #19179 has a lot of progress towards a fix (but was abandoned). Might be worth redoing those changes an incorporating a fix for this one similarly as well.

@jnvtnguyen
Copy link
Contributor

@seanlip Doesn't this just have to do with the RTE validation? I thought there were built in validators you could put in customization args and this might be related to #17491

@seanlip
Copy link
Member Author

seanlip commented Jan 24, 2024

@jnvtnguyen I do think it's related to RTE validation -- but depends which validation you're talking about. There's frontend validation and backend validation. I get the sense that the latter is fine but the former is not (hence #18211 and the fix in #19179).

github-merge-queue bot pushed a commit that referenced this issue Feb 12, 2024
…loration editor. (#19587)

* Improve Concept Card

* Fix RTE Registry

* Fix RTE Component Registry Test

* Fix Cleanup

* Change to CKEditor Function

* Fix Naming

* Fix Naming

* Naming Changes

* Fix Naming and Comments

* Fix If and Comments

* Fix Name Mismatch

* Create e2e Tests

* E2E Tests

* Fix E2E Tests

* Fix E2E

* Fix E2E

* Fix E2E

* E2E

---------

Co-authored-by: Sean Lip <sean@seanlip.org>
jayam04 pushed a commit to jayam04/oppia that referenced this issue Feb 21, 2024
…e exploration editor. (oppia#19587)

* Improve Concept Card

* Fix RTE Registry

* Fix RTE Component Registry Test

* Fix Cleanup

* Change to CKEditor Function

* Fix Naming

* Fix Naming

* Naming Changes

* Fix Naming and Comments

* Fix If and Comments

* Fix Name Mismatch

* Create e2e Tests

* E2E Tests

* Fix E2E Tests

* Fix E2E

* Fix E2E

* Fix E2E

* E2E

---------

Co-authored-by: Sean Lip <sean@seanlip.org>
kevintab95 added a commit that referenced this issue Feb 27, 2024
jnvtnguyen added a commit to jnvtnguyen/oppia that referenced this issue Feb 27, 2024
…ds in the exploration editor. (oppia#19587)"

This reverts commit c4f4ce0.
jnvtnguyen added a commit to jnvtnguyen/oppia that referenced this issue Feb 27, 2024
…e exploration editor. (oppia#19587)

* Improve Concept Card

* Fix RTE Registry

* Fix RTE Component Registry Test

* Fix Cleanup

* Change to CKEditor Function

* Fix Naming

* Fix Naming

* Naming Changes

* Fix Naming and Comments

* Fix If and Comments

* Fix Name Mismatch

* Create e2e Tests

* E2E Tests

* Fix E2E Tests

* Fix E2E

* Fix E2E

* Fix E2E

* E2E

---------

Co-authored-by: Sean Lip <sean@seanlip.org>
jnvtnguyen added a commit to jnvtnguyen/oppia that referenced this issue Feb 27, 2024
…ds in the exploration editor. (oppia#19587)"

This reverts commit c4f4ce0.
jnvtnguyen added a commit to jnvtnguyen/oppia that referenced this issue Feb 27, 2024
…e exploration editor. (oppia#19587)

* Improve Concept Card

* Fix RTE Registry

* Fix RTE Component Registry Test

* Fix Cleanup

* Change to CKEditor Function

* Fix Naming

* Fix Naming

* Naming Changes

* Fix Naming and Comments

* Fix If and Comments

* Fix Name Mismatch

* Create e2e Tests

* E2E Tests

* Fix E2E Tests

* Fix E2E

* Fix E2E

* Fix E2E

* E2E

---------

Co-authored-by: Sean Lip <sean@seanlip.org>
jnvtnguyen added a commit to jnvtnguyen/oppia that referenced this issue Feb 27, 2024
…ds in the exploration editor. (oppia#19587)"

This reverts commit c4f4ce0.
jnvtnguyen added a commit to jnvtnguyen/oppia that referenced this issue Feb 27, 2024
…e exploration editor. (oppia#19587)

* Improve Concept Card

* Fix RTE Registry

* Fix RTE Component Registry Test

* Fix Cleanup

* Change to CKEditor Function

* Fix Naming

* Fix Naming

* Naming Changes

* Fix Naming and Comments

* Fix If and Comments

* Fix Name Mismatch

* Create e2e Tests

* E2E Tests

* Fix E2E Tests

* Fix E2E

* Fix E2E

* Fix E2E

* E2E

---------

Co-authored-by: Sean Lip <sean@seanlip.org>
jnvtnguyen added a commit to jnvtnguyen/oppia that referenced this issue Feb 27, 2024
…ds in the exploration editor. (oppia#19587)"

This reverts commit c4f4ce0.
jnvtnguyen added a commit to jnvtnguyen/oppia that referenced this issue Feb 27, 2024
…e exploration editor. (oppia#19587)

* Improve Concept Card

* Fix RTE Registry

* Fix RTE Component Registry Test

* Fix Cleanup

* Change to CKEditor Function

* Fix Naming

* Fix Naming

* Naming Changes

* Fix Naming and Comments

* Fix If and Comments

* Fix Name Mismatch

* Create e2e Tests

* E2E Tests

* Fix E2E Tests

* Fix E2E

* Fix E2E

* Fix E2E

* E2E

---------

Co-authored-by: Sean Lip <sean@seanlip.org>
jnvtnguyen added a commit to jnvtnguyen/oppia that referenced this issue Feb 27, 2024
…ds in the exploration editor. (oppia#19587)"

This reverts commit c4f4ce0.
jnvtnguyen added a commit to jnvtnguyen/oppia that referenced this issue Feb 27, 2024
…e exploration editor. (oppia#19587)

* Improve Concept Card

* Fix RTE Registry

* Fix RTE Component Registry Test

* Fix Cleanup

* Change to CKEditor Function

* Fix Naming

* Fix Naming

* Naming Changes

* Fix Naming and Comments

* Fix If and Comments

* Fix Name Mismatch

* Create e2e Tests

* E2E Tests

* Fix E2E Tests

* Fix E2E

* Fix E2E

* Fix E2E

* E2E

---------

Co-authored-by: Sean Lip <sean@seanlip.org>
jnvtnguyen added a commit to jnvtnguyen/oppia that referenced this issue Feb 27, 2024
…ds in the exploration editor. (oppia#19587)"

This reverts commit c4f4ce0.
jnvtnguyen added a commit to jnvtnguyen/oppia that referenced this issue Feb 27, 2024
…e exploration editor. (oppia#19587)

* Improve Concept Card

* Fix RTE Registry

* Fix RTE Component Registry Test

* Fix Cleanup

* Change to CKEditor Function

* Fix Naming

* Fix Naming

* Naming Changes

* Fix Naming and Comments

* Fix If and Comments

* Fix Name Mismatch

* Create e2e Tests

* E2E Tests

* Fix E2E Tests

* Fix E2E

* Fix E2E

* Fix E2E

* E2E

---------

Co-authored-by: Sean Lip <sean@seanlip.org>
jnvtnguyen added a commit to jnvtnguyen/oppia that referenced this issue Feb 27, 2024
…ds in the exploration editor. (oppia#19587)"

This reverts commit c4f4ce0.
jnvtnguyen added a commit to jnvtnguyen/oppia that referenced this issue Feb 27, 2024
…e exploration editor. (oppia#19587)

* Improve Concept Card

* Fix RTE Registry

* Fix RTE Component Registry Test

* Fix Cleanup

* Change to CKEditor Function

* Fix Naming

* Fix Naming

* Naming Changes

* Fix Naming and Comments

* Fix If and Comments

* Fix Name Mismatch

* Create e2e Tests

* E2E Tests

* Fix E2E Tests

* Fix E2E

* Fix E2E

* Fix E2E

* E2E

---------

Co-authored-by: Sean Lip <sean@seanlip.org>
@jnvtnguyen
Copy link
Contributor

Completed via #19587 (Above are Accidental Commits)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Label to indicate an issue is a feature/improvement Impact: High Blocks or significantly slows down a core workflow. Work: High It's not clear what the solution is; will need investigation.
Projects
Archived in project
Development

No branches or pull requests

3 participants