-
Notifications
You must be signed in to change notification settings - Fork 25
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
Yufei - Add a mouseover text edit button near 'Total Time' #454
Conversation
This reverts commit 9b79d61. I don't need such changes not relating to my part#
Merge changes from development
How do I create a owner account? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @yufeiStella. I have left my comment in the related frontend PR. The functionality is working as intended. Great job!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @yufeiStella
Reviewed this PR with frontend PR 1030, working as expected .
Hi @yufeiStella I can confirm it works as intended. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @yufeiStella,
Reviewed this PR with frontend PR#1030, working as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I left a comment in HighestGoodNetworkApp#1030. The PR worked as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @yufeiStella, I have tested together with the Frontend, and they are working as intended
When logged in as Owner, there will be a text edit button:
Hi @yufeiStella Reviewed this PR with frontend PR 1030, working as expected . |
Tested with the front end, I have review on the front end.
256637192-cd5d1198-0e63-4f84-a35d-92400f833841.mp4 |
Hi @yufeiStella , this feature works as expected, but the page is little slow . I'm not sure, if it's because this branch or not. I'm including the screenshots below. |
|
||
return MouseoverText.findById(id, (error, mouseoverText) => { | ||
if (error || mouseoverText === null) { | ||
res.status(400).send('No mouseoverText found'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
400 means bad request, but in this case the error could be database error. I suggest use error 500.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I use error 500 now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dear @yufeiStella,
The code looks good overall. I think making minor corrections in error handling could help in further debugging. Excellent work!
|
||
return MouseoverText.findById(id, (error, mouseoverText) => { | ||
if (error || mouseoverText === null) { | ||
res.status(400).send('No mouseoverText found'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For improving error handling, I think the error 404 code could be used, along with a more intuitive error message that could aid in debugging.
Example:
res.status(404).send('MouseoverText not found with the given ID' );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated! @yurisokolovicz, could you please re-review the PR and approve it if you agree with it? Thank you
Hi @iTvX! It's all about slow loading but the change is global. For me, usually I just reload it again since it's too slow. You can also find that we are talking the problem in slack(code channel). |
This reverts commit 3e630a5. revert unnecessary commit
Merge with development
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!
Tested, everything works as expected and the code is great! |
Description
Related PRS (if any):
To test this backend PR you need to checkout the #1030 frontend PR.
Main changes explained:
How to test:
npm install
and...
to run this PR locallyScreenshots or videos of changes:
Log in as an owner:
Screen.Recording.2023-08-03.at.21.10.09.mov
Log in as any other user except owner:
Screen.Recording.2023-08-03.at.21.15.51.mov
Note:
Sometimes, you may need to wait for the page to respond and display the mouseover text. I encountered a similar situation as shown in my screenshot recording. When you move your mouse to another place where there should be mouseover text, you may experience the same situation. Just be patient and wait.