SonarCloud issues fixed #319
Conversation
There was a problem hiding this comment.
Thanks, @sp-maintenance-course !
The changes look good but can you tweak the commit and PR description to explain what and why this change was made?
Referencing SonarCloud doesn't help for folks how have never seen the report.
Maybe something like:
Updating sample HTML file to use best practices
Issues discovered via SonarCloud: https://link-if/possible
* Added HTML missing language
* Added page title
* Replaced deprecated HTML tags with css equivalent(I'm guessing SonarCloud probably has some descriptions of the issues you could use too, my comment above are just guesses based on the commit)
(I have one other question inline)
| <th>Username</th> | ||
| <th>Password</th> | ||
| <th id="username">Username</th> | ||
| <th id="password">Password</th> |
There was a problem hiding this comment.
Can you explain this change? I'm guessing if id attributes are set they should probably be set on the actual inputs?
I don't have the context of why this change is suggested, by my assumption adding the id attr base it easier to drive test frameworks like Selenium (so a getById() can be used instead of a more complicated selector?)
SonarCloud issues fixed for the samples/guice/src/main/webapp/login.jsp