mkermani144 / wanna

💡✔ Wanna is an implementation of a 21st-century to-do list app.
https://wanna.js.org
MIT License
199 stars 30 forks source link

Disable select text (#218) #252

Closed abiduzz420 closed 6 years ago

abiduzz420 commented 6 years ago

What does this PR solve ?

Why should this PR be considered?

If there any changes or modifications to be made with regards to code style guide or anything, do let me know. Thanks!

abiduzz420 commented 6 years ago

The check is failing here

(node:3921) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): WebDriverError: Authorization required
(node:3921) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
WebDriverError: Authorization required

@mkermani144 Could you help me in figuring out what could be the possible reason this error has occured?

abiduzz420 commented 6 years ago

Hi @mkermani144 I would like to know the issues which caused the TravisCI checks to fail. I am still very naive with TravisCI. I shall be happy to fix the errors :)

mkermani144 commented 6 years ago

@abiduzz420 Sorry for late response. I'm busy these days 😉. I think your PR fails because the repo E2E tests can only be run from PRs of my own repo, not any forks. I will check your PR in a few days and if it's OK I will try to fix the failure or merge it anyway.

abiduzz420 commented 6 years ago

Thank you @mkermani144 . Please let me know if you need any help.

codecov[bot] commented 6 years ago

Codecov Report

Merging #252 into master will increase coverage by 0.04%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #252      +/-   ##
==========================================
+ Coverage   82.29%   82.33%   +0.04%     
==========================================
  Files          38       39       +1     
  Lines         401      402       +1     
  Branches       32       32              
==========================================
+ Hits          330      331       +1     
  Misses         67       67              
  Partials        4        4
Impacted Files Coverage Δ
src/Task/Estimation.js 100% <ø> (ø) :arrow_up:
src/FAB/NewIdeaDialog.js 100% <ø> (ø) :arrow_up:
src/Settings/CalendarSystemDialog.js 100% <ø> (ø) :arrow_up:
src/Idea/EditIdeaDialog.js 100% <ø> (ø) :arrow_up:
src/Settings/FirstDayOfWeekDialog.js 100% <ø> (ø) :arrow_up:
src/Task/EditTaskDialog.js 100% <ø> (ø) :arrow_up:
src/Idea/ConvertIdeaDialog.js 100% <ø> (ø) :arrow_up:
src/FAB/NewTaskDialog.js 100% <ø> (ø) :arrow_up:
src/Task/Repeat.js 100% <ø> (ø) :arrow_up:
src/Task/DueDate.js 100% <ø> (ø) :arrow_up:
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8d5f880...ccd1b0e. Read the comment docs.

mkermani144 commented 6 years ago

@abiduzz420 I'm so sorry for my very late response. I fixed the problem that caused Travis to fail (see #253) and then merged the new master branch into yours. Now all your PR checks pass and there is no problem anymore. I will review your code asap, as it's not ready to be merged into the master branch and does need some major changes.

abiduzz420 commented 6 years ago

Okay sure @mkermani144 Do let me know if you need anything.

mkermani144 commented 6 years ago

@abiduzz420 In addition to my comments, I think using a global CSS style (not a JS one) such as

* {
  user-select: none;
}

in App.css will do the job without any extra effort and multiple file changes, as this is an app and no user ever need to select any text in the app most of the times (except for inputs and text areas). In addition, it adapts better to the current state of the repo, as we use CSS styles wherever possible, and switch to JS ones only when we have to.
I suggest you close this PR and make another one to prevent adding any extra commits to the repo.

mkermani144 commented 6 years ago

@abiduzz420 Please let me know if you are interested in working on #218 anymore, as I'm going to release a new version of the app and #218 needs to be fixed before the release.

abiduzz420 commented 6 years ago

Hi @mkermani144 I have a bit busy due to my exams, can I get this fixed by Sunday ?

mkermani144 commented 6 years ago

@abiduzz420 Certainly. I can wait until the end of Sunday. Please don't forget to consider the points I mentioned in my previous comment, so I can merge your next PR fast and without any change requests.

mkermani144 commented 6 years ago

Because of no response, I close this PR. To fix the issue I opened #264.