-
Notifications
You must be signed in to change notification settings - Fork 100
Fixes issue #69 : Enhanced feedback to Selection/Deselection of CheckBox #71
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
Conversation
Reviewer's Guide by SourceryThis pull request enhances the user experience by providing visual feedback when checkboxes are selected or deselected. It modifies the JavaScript code to toggle CSS classes on the associated label elements, and it adds new CSS classes to define the styles for the selected and deselected states. A transition effect was added for a smoother user experience. Updated class diagram for label elementsclassDiagram
class HTMLLabelElement {
+string className
+add(string className)
+remove(string className)
}
note for HTMLLabelElement "Added 'selectedLabel' and 'unselectedLabel' classes to provide visual feedback"
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 @Preeti9764 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using a more descriptive class name than
selectedLabelandunselectedLabel. - It looks like the
handleOpenLabelChangefunction is setting the same value for bothshowOpenLabelandshowClosedLabel- is that intentional?
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
| function handleLastWeekContributionChange() { | ||
| var value = lastWeekContributionElement.checked; | ||
| var labelElement = document.querySelector("label[for='lastWeekContribution']"); |
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.
issue (complexity): Consider creating a helper function to toggle the label classes to reduce code duplication in the handleLastWeekContributionChange and handleOpenLabelChange functions
You can reduce duplication by extracting the label toggling logic into a helper function. For example:
function updateLabelClass(labelElement, isSelected) {
labelElement.classList.toggle("selectedLabel", isSelected);
labelElement.classList.toggle("unselectedLabel", !isSelected);
}Then update your handlers to use the helper:
function handleLastWeekContributionChange() {
var value = lastWeekContributionElement.checked;
var labelElement = document.querySelector("label[for='lastWeekContribution']");
if (value) {
startingDateElement.disabled = true;
endingDateElement.disabled = true;
endingDateElement.value = getToday();
startingDateElement.value = getLastWeek();
updateLabelClass(labelElement, true);
handleEndingDateChange();
handleStartingDateChange();
} else {
startingDateElement.disabled = false;
endingDateElement.disabled = false;
updateLabelClass(labelElement, false);
}
chrome.storage.local.set({ lastWeekContribution: value });
}
function handleOpenLabelChange() {
var value = showOpenLabelElement.checked;
var labelElement = document.querySelector("label[for='showOpenLabel']");
updateLabelClass(labelElement, value);
chrome.storage.local.set({ showOpenLabel: value });
}This removes duplicated code while keeping functionality intact.
|
@hongquan hey can you review my PR and guide me on any further changes required, I find this project really interesting and would love to contribute further. |
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.
@mariobehling @hongquan Please have a look and suggest some changes and guide for further improvement in UI .
vedansh-5
left a comment
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.
fix the mentioned calls and the PR is good to be merged.
| endingDateElement.disabled = true; | ||
| endingDateElement.value = getToday(); | ||
| startingDateElement.value = getLastWeek(); | ||
| handleEndingDateChange(); |
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.
You seem to have removed these function calls from handleLastWeekContributionChange().
These are required to change the dates accordingly.
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.
Thanks for catching that! I've re-added the handleEndingDateChange() and handleStartingDateChange() calls.
vedansh-5
left a comment
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.
lgtm! Thanks @Preeti9764
Scrum subject and body message are now correctly added in Outlook (fossasia#81)
|
@mariobehling, please have a look at this pr and suggest to me further changes I could do |
Fixes issue #69
Changes:
Summary by Sourcery
Enhance user feedback for checkbox interactions by adding visual styling to improve UI responsiveness
Bug Fixes:
Enhancements: