-
Notifications
You must be signed in to change notification settings - Fork 192
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
To do feature added #258
base: master
Are you sure you want to change the base?
To do feature added #258
Conversation
please review... |
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.
Use bootstrap 4 so that it will be in sync to rest all pages and better responsiveness.
Todo.html
Outdated
<head> | ||
<meta charset="UTF-8"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
<!-- <title>Todo App JavaScript--> |
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.
Keep the header as TODO List
<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/5.15.3/css/all.min.css"/> | ||
</head> | ||
<body> | ||
<div class="wrapper"> |
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.
Use bootstrap container instead of writing whole new styles for the wrapper.
<body> | ||
<div class="wrapper"> | ||
<header>Todo App</header> | ||
<div class="inputField"> |
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.
Use bootstrap form styles.
<button>Clear All</button> | ||
</div> | ||
</div> | ||
|
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.
Keep the footer that is present on the home page.
</head> | ||
<body> | ||
<div class="wrapper"> | ||
<header>Todo App</header> |
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.
Add the navbar that is present on the home page.
height: 100vh; | ||
/* overflow: hidden; */ | ||
padding: 10px; | ||
background: linear-gradient(to bottom, #68EACC 0%, #497BE8 100%); |
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.
Pick colours from the existing scheme, this gradient looks far off.
//if the user value isn't only spaces | ||
addBtn.classList.add("active"); //active the add button | ||
} else { | ||
addBtn.classList.remove("active"); //unactive the add button |
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.
Try adding the disabled bootstrap class so the cursor also gets changed.
js/todo.js
Outdated
addBtn.onclick = () => { | ||
//when user click on plus icon button | ||
let userEnteredValue = inputBox.value; //getting input field value | ||
let getLocalStorageData = localStorage.getItem("New Todo"); //getting localstorage |
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.
Make the key more unique something like algo-phantoms-todo-list
Few requested changes are missing. @ShambhaviSharma0110 |
Description
Please include a summary of the change and which issue is fixed.
Fixes #190
Have added 3 separate files of HTML,CSS, and js and have integrated them under the more dropdown section in the navbar.
List any dependencies that are required for this change
none
Type of change
How Has This Been Tested?
UI /UX changes
Attach gif or screenshot for changes.
- After :
Checklist: