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

feat(elm): Add form for creating a category #76

Merged
merged 3 commits into from
Aug 16, 2017
Merged

Conversation

pmrukot
Copy link
Collaborator

@pmrukot pmrukot commented Aug 16, 2017

Summary

  • Adds a form to create a new category
  • Currently just supports one-word uppercase category names (should we change it?)

Related issues
Closes #41

Test plan
Just test the form

Copy link
Collaborator

@mrapacz mrapacz left a comment

Choose a reason for hiding this comment

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

I think the regex part could work better here, especially the Unicode. The rest is highly analogous to #61, which means it's correct, but cannot wait for #56

import Room.Models exposing (RoomsData)


-- create question section


createQuestionUrl : String
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can move the Urls to Constants.elm

import Toasty.Defaults


categoryFormValidationErrorToast : ( Model, Cmd Msg ) -> ( Model, Cmd Msg )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here I have a similar comment as to the previous PR. If we've put three functions mirroring three types of notifications in src/Notifications.elm, then this file could only consist of three strings + import Notifications. I think that would make it simpler, but, as I said, we can delay it until we'll be dealing with global elm refactor.

categoryNameValidations =
let
categoryNameRegex =
"\\b([A-Z][a-z]+)\\b"
Copy link
Collaborator

Choose a reason for hiding this comment

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

^\s*(.+?)\s+$

You could do this for multi word categories. Did you check how your regex behaves when dealing with Unicode characters?

@@ -50,6 +52,24 @@ update msg model =
_ ->
model ! []

OnCategoryCreated response ->
case response of
RemoteData.Success responseData ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

This just cries for a proper setter method, cannot wait for #56

@pmrukot
Copy link
Collaborator Author

pmrukot commented Aug 16, 2017

Updated branch and adjusted code to your suggestions. Lets leave regex discussion for another day..

@mrapacz
Copy link
Collaborator

mrapacz commented Aug 16, 2017

giphy 1

@pmrukot pmrukot merged commit f534639 into master Aug 16, 2017
@pmrukot pmrukot deleted the feat/create-category branch August 16, 2017 21:31
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.

2 participants