Implement text prediction functionality#10
Implement text prediction functionality#10dmrib wants to merge 40 commits intomachinegrooving:masterfrom
Conversation
HeitorBoschirolli
left a comment
HeitorBoschirolli
left a comment
There was a problem hiding this comment.
I'm too sleepy to continue. Stopped at "Add uuid as a dependency". Will finish tomorrow
| // on connection closed event callback | ||
| socket.current.onclose = (event) => | ||
| { | ||
| console.log("Websocket connection closed."); |
There was a problem hiding this comment.
I think we should log event.code as well. This way we can differentiate connections closed normally unexpected behavior. And we should try to reconnect if the WebSocket closes
suggestion:
if (event.code === 1000)
console.log('Websocket connection closed cleanly');
else
{
console.log(`Websocket connection closed uncleanly. code: ${event.code}, trying to reconnect`);
socket.current = new WebSocket(CONNECTION_ADDRESS);
}
| <Popover | ||
| content={<p style={{ color: '#a7425c' }}>Coming soon</p>} | ||
| trigger="hover" | ||
| > |
There was a problem hiding this comment.
the popover's direction is incorrect. Pay attention to the bottom part of the popover in the image attached.
| return( | ||
| <Popover | ||
| content={<p style={{ color: '#a7425c' }}>Coming soon</p>} | ||
| content={ |
There was a problem hiding this comment.
Here you're changing functionality added in this PR. Ideally, we would squash some commits or split this into two PRs. Do you think it's worth the trouble?
| color: '#a7425c' | ||
| }} | ||
| > | ||
| Prediction size: |
There was a problem hiding this comment.
It would be cool to show the current prediction size. We open an issue for this if you prefer.
| "marked": "^0.8.0", | ||
| "react": "^16.12.0", | ||
| "react-dom": "^16.12.0", | ||
| "react-redux": "^7.2.0", |
There was a problem hiding this comment.
From this point forward it definitely seems like it should be a new PR. Again, do you think it's work the trouble? It could be a single PR with two version updates as well
| */ | ||
| function setMaxSize(size) { | ||
| return { | ||
| type: 'SET_MAX_SIZE', |
There was a problem hiding this comment.
Do you think there's benefit in centralizing the actions strings in a separate file as constants so we always use the same thing in actions and reducers?
for example:
actions/types.js
export SET_MAX_SIZE = 'SET_MAX_SIZE';
actions/predictions.js
import {SET_MAX_SIZE} from '../types';
...
type: SET_MAX_SIZE
...
reducers/predictions.js
import {SET_MAX_SIZE} from '../../actions/types';
...
case SET_MAX_SIZE:
...
Advantages I see: no errors caused by typos related to action names.
Disadvantages I see: more lines of code and a more complex file structure.
| * @component | ||
| */ | ||
| function Content() | ||
| function Content({socket}) |
There was a problem hiding this comment.
The WebSocket could be in the Store
2 participants
No description provided.