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.

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

Add theme switch #105

Merged
merged 3 commits into from
Oct 4, 2022
Merged

Add theme switch #105

merged 3 commits into from
Oct 4, 2022

Conversation

Mathanraj-Sharma
Copy link
Member

Fix #77

@Mathanraj-Sharma Mathanraj-Sharma added area/ui UI related issue hacktoberfest Issues suited for hacktoberfest. labels Oct 1, 2022
@Mathanraj-Sharma Mathanraj-Sharma self-assigned this Oct 1, 2022
Copy link
Collaborator

@mturoci mturoci left a comment

Choose a reason for hiding this comment

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

Thanks @Mathanraj-Sharma! A few small comments from me, looks good otherwise.

explaining-ratings/src/app.py Outdated Show resolved Hide resolved
Comment on lines 119 to 134
await update_theme(q)
else:
del q.page['diff']
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reason for including the rest of the handlers in an else block? Can it be flattened (else removed)?

Copy link
Member Author

Choose a reason for hiding this comment

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

if they are not in else block when we switch them it resubmits the filters as well. So the rating diff also getting refreshed

Copy link
Collaborator

Choose a reason for hiding this comment

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

But if the same filters are applied again, shouldn't it result in the very same diff? In which case it doesn't matter if they are rerun or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mturoci no it doesn't, it creates different word clouds (as diff) for each time it gets triggered with the same filters

@Mathanraj-Sharma
Copy link
Member Author

@mturoci I have improved the conditions to switch theme, please check

@mturoci mturoci merged commit 7bab955 into main Oct 4, 2022
@mturoci mturoci deleted the mathan/fix/issue-77 branch October 4, 2022 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui UI related issue hacktoberfest Issues suited for hacktoberfest.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explainable Hotel ratings - App doesn't have an option for Dark Mode (optional)
2 participants