Skip to content

Style tweaks: notification on toggling style tweak with a gesture #10674

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

Merged
merged 3 commits into from
Jul 12, 2023

Conversation

hius07
Copy link
Member

@hius07 hius07 commented Jul 11, 2023

Closes #10667.


This change is Reviewable

@@ -726,6 +727,7 @@ function ReaderStyleTweak:onToggleStyleTweak(tweak_id, item)
else
self.doc_tweaks[tweak_id] = nil
end
text = _("Disabled style tweak: ")
Copy link
Contributor

Choose a reason for hiding this comment

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

@Frenzie will confirm if there is a risk having translatable strings ending with a space - and translator missing it in their translation.

(These notifications will probably be long and end up truncated.)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, at a glance it'd be better to use the template function right here and in the else with %1 and self.tweaks_in_dispatcher[tweak_id] as an argument.

@@ -727,7 +727,7 @@ function ReaderStyleTweak:onToggleStyleTweak(tweak_id, item, no_notification)
else
self.doc_tweaks[tweak_id] = nil
end
text = _("Disabled style tweak: ")
text = T(_("Off: %1"), self.tweaks_in_dispatcher[tweak_id])
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that for such short and standard On/Off:, we should add some translation context and/or a @ translator comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we just make these notifications always in two lines? First line is “Style tweak enabled/disabled:” in any language, second line is the css name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently notification is a TextWidget

A TextWidget puts a string on a single line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I have thought about having it multilines in the past, but gave up. We don't really need it to be more than one line, we're mostly fine elsewhere - and most people don't care about notifications, so the smaller the better :)

Copy link
Member

Choose a reason for hiding this comment

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

I do think it's a good suggestion all around. Concatenating .. "\n" .. (or maybe two) bothers translators the minimum amount and the display would likely be better too. Except for that single line notification aspect. :-)

Copy link
Member

Choose a reason for hiding this comment

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

But yes, also notifications should be as small as possible, forgot about that for a second.

@Frenzie Frenzie added this to the 2023.07 milestone Jul 11, 2023
Copy link
Contributor

@poire-z poire-z left a comment

Choose a reason for hiding this comment

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

We'll see how many people have a profile enabling 14 tweaks and get a notification stack overflow :)

@mergen3107
Copy link
Contributor

Stack Overflow on GitHub? That’s something new :D

@hius07 hius07 merged commit e1ed3a7 into koreader:master Jul 12, 2023
@hius07 hius07 deleted the style-tweak-notification branch July 12, 2023 04:45
@mergen3107
Copy link
Contributor

Thank you @hius07 !
Works like a charm. The only suggestion I’d make is to add “.css” to make clear that this is a style tweak, and not a profile, for example.

Otherwise works great and now I have a visual confirmation :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: Add notification when Style Tweak is triggered via gestures
4 participants