Skip to content
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.

Sign up for GitHub

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

Saqib #214

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Saqib #214

Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a41ec9f
Update todo.ejs
Sachin-arivani Sep 23, 2024
9a9d6e2
Update todo.ejs
Sachin-arivani Sep 23, 2024
b4f8e96
Update todo.ejs
Sachin-arivani Sep 23, 2024
d5030f4
Update todo.ejs
Sachin-arivani Sep 23, 2024
f04a3c3
Update todo.ejs
Sachin-arivani Sep 23, 2024
c0b31ff
Update todo.ejs
Sachin-arivani Sep 23, 2024
968c615
Update todo.ejs
Sachin-arivani Sep 23, 2024
8c545c8
Update todo.ejs
Sachin-arivani Sep 23, 2024
181340e
Update todo.ejs
Sachin-arivani Sep 23, 2024
89e7424
Update todo.ejs
Sachin-arivani Sep 23, 2024
f983a81
Update todo.ejs
Sachin-arivani Sep 23, 2024
3410e93
Update todo.ejs
Sachin-arivani Sep 23, 2024
9dcc061
Update todo.ejs
Sachin-arivani Sep 23, 2024
c1b796d
Update todo.ejs
Sachin-arivani Sep 23, 2024
3020444
Update todo.ejs
Sachin-arivani Sep 23, 2024
cead175
Update todo.ejs
Sachin-arivani Sep 25, 2024
a921250
testing from local
Sep 25, 2024
9060a9c
saqib pushed code
mohdsaqib12 Sep 25, 2024
9f8fbaa
saqib branch
mohdsaqib12 Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions views/todo.ejs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
}

h1 {
background-color: #4CAF50;
background-color: green;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

LGTM, but consider consistency in color definitions.

The change from #4CAF50 to green is acceptable. However, for better consistency and precise control over the color scheme, consider using hex color codes throughout the stylesheet.

If you decide to keep using named colors, ensure this approach is applied consistently across the entire stylesheet for better maintainability.

color: white;
margin: 0;
padding: 20px;
Expand Down Expand Up @@ -89,7 +89,7 @@
</head>

<body>
<h1>Hello GNA University, Write your Goals Below</h1>
<h1>Sachin Sharma.</h1>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Clarify the purpose of the header change and resolve inconsistencies.

The header content has been significantly altered, which raises several concerns:

  1. The instructional text "Write your Goals Below" has been removed, which might impact user understanding of the page's purpose.
  2. There's an inconsistency between the PR author (mohdsaqib12) and the name in the header (Sachin Sharma).
  3. The change from a general greeting to a specific name might not be suitable for a shared application.

Please clarify the following:

  1. Is this change intentional, and if so, what's the reasoning behind it?
  2. Should the header contain the PR author's name instead?
  3. How will users understand the purpose of the page without the instructional text?

Consider reverting to the original text or providing a more general header that maintains the app's purpose.

<ul>
<% todolist.forEach(function(todo, index) { %>
<li>
Expand Down