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

Disable editing vocabulary word #38920

Merged
merged 4 commits into from
Feb 5, 2021
Merged

Conversation

bethanyaconnor
Copy link
Contributor

Follow up to this Slack discussion. We currently set the key of vocab equal to the word to 1. prevent duplicate definitions (for now) and 2. to make the vocab markdown syntax intuitive. This means that if a vocab word is edited, its key will be out of sync with these objectives.

The biggest downside to this solution is that if there's a typo in the word, it will be difficult to edit without creating a new object. I added a warning to that effect (open to suggestion on wording!) but this approach might need to be revisited if we hear feedback about this.

Updated add vocab dialog:
Screen Shot 2021-02-04 at 4 01 38 PM

Updated edit dialog:
Screen Shot 2021-02-04 at 3 56 09 PM

Links

Testing story

Reviewer Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@bethanyaconnor
Copy link
Contributor Author

Also tagging @tess323 mostly for fyi but in case she has any objections!

@tess323
Copy link

tess323 commented Feb 5, 2021

Thanks for the heads up! Only feedback would be to make that alert red if its a low lift

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Looks great!

@dmcavoy
Copy link
Contributor

dmcavoy commented Feb 5, 2021

@bethanyaconnor Does having word match the key prevent CSF from having two of the same words with different definitions for the common sense media lessons?

@bethanyaconnor
Copy link
Contributor Author

Currently yes. I thought we were going to address that after April launch but I might've misunderstood something. Ideally I'd like a way to generate a key that makes sense for those vocabulary, i.e. debugging_cse or something.

FWIW I don't think it will be much work to implement this now if it's needed now.

@davidsbailey
Copy link
Member

FWIW I don't think it will be much work to implement this now if it's needed now.

I'm open to this. What would this look like exactly in terms of changes that need to be made?

@bethanyaconnor
Copy link
Contributor Author

Kind of depends how we want the UI to look. I've been envisioning a checkbox or something to mark a vocabulary as common sense media. Then modifying this function to take that into account. Then maybe disallowing editing for common sense media vocabulary (as that seems to be the requirement).

I didn't add properties to the vocabulary model because it wasn't needed originally but that would be an easy migration. But even with that, I think this would be less than a day of work.

@davidsbailey
Copy link
Member

It could be worth considering if it helps us in the short term -- otherwise, if we're not quite sure how we want it to look yet, I would probably lean toward deferring it.

@bethanyaconnor
Copy link
Contributor Author

I created a ticket in our backlog for that work so we can talk about it at planning. It isn't directly related to this PR though so I'm going to go ahead and merge this.

@bethanyaconnor bethanyaconnor merged commit 0314d3b into staging Feb 5, 2021
@bethanyaconnor bethanyaconnor deleted the disable-vocab-word-edit branch February 5, 2021 23:39
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.

None yet

4 participants