Finalize Article Editor v2 #522
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The only thing this commit does is run prettier on files we'll be modifying to update the <Tags /> component.
I had to move algoliasearch setup to the constructor. Otherwise, the setup happens when you import ArticleForm. Feel free to let me know if there's a better way.
This test adds a mock for algoliasearch. We trigger handleTagKeyUp to test that tagList and tagOptions are updated once the mock search returns.
There are two tests so far. 1. You can click on a tag to add it to the list. 2. You can type a comma which will also add the tag to the list.
I believe <Tags> is meant to be a controlled component. Using onKeyUp sort've acts as both uncontrolled and controlled. The value of the textarea has already been changed in the DOM, then we try to change it again using setState. This leads to some weird issues where what you type gets mangled. For example, you may type "javascript", but end up with something like "jvascrit". Switching this to onInput lets us control everything and gets rid of the weird issue of missing characters as you type.
We add tags when you click on a search result or hit enter on a search result. I'm doing this because I think it will help insert tags at any location in the textarea. Currently, it only supports adding tags to the end of the list.
If you use the arrow keys or the mouse to go back to edit a tag you've typed, it'd be nice if the search results could replace that tag. Before this, if you click or hit enter on a search result, the tag gets added to the end of the list. This commit will replace the tag you're editing.
This was causing anything inside __mocks__ to be considered as a test.
1. That we query for the correct tags based on what we're editing 2. That we replace the correct tag when we choose a new one
Extracts building of <ArticleForm /> component since we're using it in a number of places for test setup.
There appears to be an issue with the input losing the cursor position when it rerenders. I'm not sure if this is an issue with the code or an issue with preact. I'm leaning toward an issue with the code, but I'm not really sure.
|
Hey, I think the TODO is very logical. Since this feature remains hidden in production, feel free to take the WIP tag off as soon as you feel like the code is in a decent place and we can try and get it merged in its current state. |
This is to prep for moving any tags related behavior from <ArticleForm /> to <Tags />.
The <Tags /> component seems like the right place to perform searching and managing of selected tags. We can pass a function to <Tags /> so the tag list can be updated when needed.
We already have `props.defaultValue` which is already a kind of "state" for selected tags. Keeping track of an array of selected tags seems like overkill here. We can transform the defaultValue string to a list of selected tags when needed.
We're already using linkState elsewhere so I think it makes sense to use it here as well.
We were performing quite a bit of logic when handling key down events. This commit extracts this logic to small methods. For example, we need to check if we're at the top or bottom of the search results. We also need to clear search results and reset the selected search result. My hope is to make this logic a little bit clearer with methods named by what they do.
If the query is blank, it seems to make sense that we'd clear search results inside the search method. That's an empty query and the search method is simply returning no search results in that case. However, if we don't wrap the resetting of search results with a Promise, we end up with a double render where the cursor moves to the end of the textarea. The Promise feels a little hacky to me, but I couldn't come up with something better.
This would've been hard to understand in the future. I named the conditionals a little bit better and extracted logic to insert a space at a given position.
This now uses the getter for selected tags as a filter.
|
Thanks @benhalpern. I think my changes are a bit better now. I pushed much of the tags behavior down to the I'm taking the WIP tag off now. Let me know what you think and any changes/updates you think we should make here. Also, I'll keep an eye out for any questions about changes I made or new things I added. There might be better ways to do some of the things I did. |
benhalpern
approved these changes
Sep 12, 2018
isogram
pushed a commit
to isogram/dev.to
that referenced
this pull request
Oct 15, 2018
* Run prettier on articleForm.jsx and tags.jsx The only thing this commit does is run prettier on files we'll be modifying to update the <Tags /> component. * Add basic snapshot test for ArticleForm I had to move algoliasearch setup to the constructor. Otherwise, the setup happens when you import ArticleForm. Feel free to let me know if there's a better way. * Add a snapshot test to exercise algoliasearch This test adds a mock for algoliasearch. We trigger handleTagKeyUp to test that tagList and tagOptions are updated once the mock search returns. * Add test for selecting tags There are two tests so far. 1. You can click on a tag to add it to the list. 2. You can type a comma which will also add the tag to the list. * Add constants for key codes * Change onKeyUp to onInput when typing tags I believe <Tags> is meant to be a controlled component. Using onKeyUp sort've acts as both uncontrolled and controlled. The value of the textarea has already been changed in the DOM, then we try to change it again using setState. This leads to some weird issues where what you type gets mangled. For example, you may type "javascript", but end up with something like "jvascrit". Switching this to onInput lets us control everything and gets rid of the weird issue of missing characters as you type. * Refactor the way we add tags to the input We add tags when you click on a search result or hit enter on a search result. I'm doing this because I think it will help insert tags at any location in the textarea. Currently, it only supports adding tags to the end of the list. * Allow editing of tag based on cursor position If you use the arrow keys or the mouse to go back to edit a tag you've typed, it'd be nice if the search results could replace that tag. Before this, if you click or hit enter on a search result, the tag gets added to the end of the list. This commit will replace the tag you're editing. * Move __mocks__ outside of __tests__ This was causing anything inside __mocks__ to be considered as a test. * Add 2 tests around editing tags 1. That we query for the correct tags based on what we're editing 2. That we replace the correct tag when we choose a new one * Enforce max tags limit * Refactor article form tests Extracts building of <ArticleForm /> component since we're using it in a number of places for test setup. * Add a space after inserting a tag There appears to be an issue with the input losing the cursor position when it rerenders. I'm not sure if this is an issue with the code or an issue with preact. I'm leaning toward an issue with the code, but I'm not really sure. * Refactor <Tags /> to class This is to prep for moving any tags related behavior from <ArticleForm /> to <Tags />. * Move tags behavior to <Tags /> component The <Tags /> component seems like the right place to perform searching and managing of selected tags. We can pass a function to <Tags /> so the tag list can be updated when needed. * Move <Tags /> selected state to a getter We already have `props.defaultValue` which is already a kind of "state" for selected tags. Keeping track of an array of selected tags seems like overkill here. We can transform the defaultValue string to a list of selected tags when needed. * Use linkState to manage tagList We're already using linkState elsewhere so I think it makes sense to use it here as well. * Extract key handling behavior to methods We were performing quite a bit of logic when handling key down events. This commit extracts this logic to small methods. For example, we need to check if we're at the top or bottom of the search results. We also need to clear search results and reset the selected search result. My hope is to make this logic a little bit clearer with methods named by what they do. * Move clearing of search results into search method If the query is blank, it seems to make sense that we'd clear search results inside the search method. That's an empty query and the search method is simply returning no search results in that case. However, if we don't wrap the resetting of search results with a Promise, we end up with a double render where the cursor moves to the end of the textarea. The Promise feels a little hacky to me, but I couldn't come up with something better. * Refactor naming of logic to insert a space following a comma This would've been hard to understand in the future. I named the conditionals a little bit better and extracted logic to insert a space at a given position. * Fix issue with duplicates appearing in search results This now uses the getter for selected tags as a filter.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this? (check all applicable)
Description
First of all, thanks to @maestromac. It took me awhile to figure out the existing behavior, but I think I was able to understand most of it by reading through the code.
In 727d145, I ran prettier on the files in question so the future commits wouldn't get too noisy. It might make sense to make a separate PR just for that, then this PR has the relevant diff (just let me know).
With @benhalpern's help and my own investigations, I think I have the behavior understood and where we're trying to go with this. However, I'd consider this still very much a WIP.
Notable issues fixed so far:
onKeyUptoonInputhelped fix this.Things I'd still like to do:
<Tags />component. I think that's where it should go, but let me know if my thinking is off. I also haven't refactored hardly at all - just trying to get things under test with the right behavior.Related Tickets & Documents
Closes #272
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Added to documentation?