Skip to content

feat(Notifications): Adjust alert layout to show more text#1422

Open
FrazerClews wants to merge 1 commit intoInfiniTimeOrg:mainfrom
FrazerClews:notificationAlertChange
Open

feat(Notifications): Adjust alert layout to show more text#1422
FrazerClews wants to merge 1 commit intoInfiniTimeOrg:mainfrom
FrazerClews:notificationAlertChange

Conversation

@FrazerClews
Copy link
Copy Markdown

Original: #1348

To make better use of the space available in the header, it makes sense to increase the width of the alert_type.

To avoid confusion with the alert_counter, a will separate the alert_counter from the alert_type.

I do have a branch ready which furthers this PR by making better use of the height of the header space as there is a lot of space. This allows more text in the subject content, but this may cause accessibility issues.

before
after

@FrazerClews
Copy link
Copy Markdown
Author

@NeroBurner

nice improvement with minimal change, I like it

Improving the scroll speed should be done in a new PR to keep this one minimal

The firmware build is failing because of the branch name containing a / character, the PR itself seems fine

Thanks, I would also prefer to increase the scrolling speed, but not sure if it would be too fast for some people. I am happy to increase it in this PR though.

@FrazerClews
Copy link
Copy Markdown
Author

FrazerClews commented Nov 7, 2022

@NeroBurner

nice improvement with minimal change, I like it
Improving the scroll speed should be done in a new PR to keep this one minimal
The firmware build is failing because of the branch name containing a / character, the PR itself seems fine

Thanks, I would also prefer to increase the scrolling speed, but not sure if it would be too fast for some people. I am happy to increase it in this PR though.

After doing some tests, I remembered experimenting with this a while back. One problem I found was that by the time notification loads, a good portion of the text at the beginning has rotated and unreadable.

I tried looking for a way to maybe pause the alert text for a second then start the the circular rotation, but there is no built functionality in LVGL 7.11 for that from what I have seen.

@NeroBurner NeroBurner added the enhancement Enhancement to an existing app/feature label Nov 7, 2022
@NeroBurner NeroBurner added this to the 1.12.0 milestone Nov 7, 2022
@NeroBurner
Copy link
Copy Markdown
Contributor

Nice, now the CI is all green ✔️

I'm not against the scroll speed improvement, but I advice to do it in a different PR to keep this one minimal and easy to review. By mashing different improvements into one PR the PR gets bigger, harder to review. And more people with differing opinions, which could lead to longer time to reach a consensus, blocking the other improvements to be merged. So small PRs improving one thing are a good thing in my opinion :)

@FrazerClews
Copy link
Copy Markdown
Author

Sorry, not too sure why I read your previous message as including animation speed up in this PR, but I agree to keep them separate.

@JF002
Copy link
Copy Markdown
Collaborator

JF002 commented Dec 27, 2022

The additional space for the notification title is more than welcome. However, I'm not completely satisfied by the | separator... I don't know how to explain it, but it doesn't feel right.
Maybe we could experiment with LVGL object mask to some kind of shading effect at the end of the label, for example?

@JF002 JF002 removed this from the 1.12.0 milestone Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Enhancement to an existing app/feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants