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

Allow experience domains creation in task creation #4119

Merged
merged 6 commits into from
Jun 13, 2019

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Jun 1, 2019

This PR makes it possible to add new experience domains while creating tasks.

URL of deployed dev instance (used for testing):

Steps to test:

  • Open the task creation view
  • Try to select an experience domain, that did not exist before.
  • Also, open the admin's users view and try to change the experience domain of a user. It should work exactly as before.

Issues:


@MichaelBuessemeyer MichaelBuessemeyer self-assigned this Jun 1, 2019
@MichaelBuessemeyer MichaelBuessemeyer changed the title [WIP] Allow experience domains creation in task creation Allow experience domains creation in task creation Jun 3, 2019
@@ -410,7 +410,7 @@ class ExperienceModalView extends React.PureComponent<Props, State> {
) : null}
<SelectExperienceDomain
disabled={false}
mode="tags"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mode property is no longer needed here. It served the purpose to be able to select the entered text/experience domain although it is not included in the suggestions. As I implemented this behaviour manually, this is no longer required. Please note that the approach "changing mode to 'tags' in task_create_form_view" does change the behaviour of the select and thus this is not an option for the task creation view.

@MichaelBuessemeyer
Copy link
Contributor Author

@daniel-wer Could I please get your review on this PR?

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Sry, this slipped through the cracks :/

Works like a charm, please see my small suggestion 👍

!options.includes(currentlyEnteredDomain) &&
currentlyEnteredDomain.trim() !== ""
) {
options.push(currentlyEnteredDomain);
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit hesitant about the modification of options here. Currently it's not a problem as state.domains is filtered in getUnusedDomains and returns a new array, but this could easily change in the future. This could then result in a hard to find state modification error.
Maybe change options to a let and then do options = [...options, currentlyEnteredDomain];

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for paying attention to this 👍

@MichaelBuessemeyer MichaelBuessemeyer merged commit 77c6140 into master Jun 13, 2019
@normanrz normanrz deleted the allow-exp-domain-creation branch August 12, 2019 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to create new exp domain when creating a task
2 participants