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

[newsfeed] Suppress unsightly animation edge cases when there are 0 or 1 active news items #3336

Merged

Conversation

crazyscot
Copy link
Contributor

When the newsfeed module has an items list of size 1, every updateInterval the animation runs to transition from the active story to itself. This is unsightly. This PR suppresses that.

To reproduce: configure newsfeed with a single news source, ignoreOldItems true, a short updateInterval (e.g. 3000), and a carefully-chosen small ignoreOlderThan lining up with the current contents of your news source.

@rejas
Copy link
Collaborator

rejas commented Jan 7, 2024

Thanks for the PR. Quick question though: if the newsfeed starts with 1 item only, and has more items only a while later, this will still update as expected?

@crazyscot
Copy link
Contributor Author

node_helper sends a NEWS_ITEMS notify, this calls generateFeed which updates this.newsItems.
Next updateInterval, newsItems.length > 1, no problem.

However I think I have spotted an edge case: if there is still precisely one item, but a different item, the display won't be updated.

An obvious workaround is to forcibly redraw the DOM after calling generateFeed(), but I think that will cause the same unsightliness, alebit just at a lower rate.
There may be a better workaround by special-case cacheing the currently displayed item, but it feels messy?

@rejas
Copy link
Collaborator

rejas commented Jan 7, 2024

However I think I have spotted an edge case: if there is still precisely one item, but a different item, the display won't be updated.

how about storing a hash value of the single item and compare that against the next?

@crazyscot crazyscot force-pushed the bugfix/newsfeed-animation-single-item branch from ada2bb3 to 59ac873 Compare January 13, 2024 04:32
@crazyscot
Copy link
Contributor Author

crazyscot commented Jan 13, 2024

I have thought this through a bit more and realised there was a further animation to suppress: the case where there are 0 active items. This PR now catches all of them; see the detailed comment in newsfeed.js.

  • Branch rebased onto the tip of develop.
  • The branch also includes two minor bugfix commits in getTemplateData; they weren't affecting me, but they jumped out when I was working there.

@crazyscot crazyscot changed the title [newsfeed] Suppress animation when there is only a single news item [newsfeed] Suppress unsightly animation edge cases when there are 0 or 1 active news items Jan 13, 2024
@crazyscot crazyscot force-pushed the bugfix/newsfeed-animation-single-item branch from 59ac873 to 3b92514 Compare January 13, 2024 22:12
@crazyscot
Copy link
Contributor Author

Fixed linter issues.

@rejas rejas merged commit dadc7ba into MagicMirrorOrg:develop Jan 14, 2024
5 checks passed
@crazyscot crazyscot deleted the bugfix/newsfeed-animation-single-item branch January 14, 2024 09:33
rejas pushed a commit that referenced this pull request Jan 18, 2024
It appears that #3336 introduced a bug where a newsfeed with >1 items
would stop updating after a while (usually after `activeItem` wraps
around the end of the list). Sorry! My bad, I hadn't tested that case
well enough.
@rejas rejas mentioned this pull request Apr 1, 2024
rejas added a commit that referenced this pull request Apr 1, 2024
## [2.27.0] - 2024-04-01

Thanks to: @bugsounet, @crazyscot, @illimarkangur, @jkriegshauser,
@khassel, @KristjanESPERANTO, @Paranoid93, @rejas, @sdetweil and
@vppencilsharpener.

This release marks the first release without Michael Teeuw (@MichMich).
A very special thanks to him for creating MagicMirror and leading the
project for so many years.

For more info, please read the following post: [A New Chapter for
MagicMirror: The Community Takes the
Lead](https://forum.magicmirror.builders/topic/18329/a-new-chapter-for-magicmirror-the-community-takes-the-lead).

### Added

- Output of system information to the console for troubleshooting (#3328
and #3337), ignore errors under aarch64 (#3349)
- [chore] Add `eslint-plugin-package-json` to lint the `package.json`
files (#3368)
- [weather] `showHumidity` config is now a string describing where to
show this element. Supported values: "wind", "temp", "feelslike",
"below", "none". (#3330)
- electron-rebuild test suite for electron and 3rd party modules
compatibility (#3392)
- Create MM² icon and attach it to electron process (#3407)

### Updated

- Update updatenotification (update_helper.js): Recode with pm2 library
(#3332)
- Removing lodash dependency by replacing merge by spread operator
(#3339)
- Use node prefix for build-in modules (#3340)
- Rework logging colors (#3350)
- Update pm2 to v5.3.1 with no allow-ghsas (#3364)
- [chore] Update husky and let lint-staged fix ESLint issues
- [chore] Update dependencies including electron to v29 (#3357) and
node-ical
- Update translations for estonian (#3371)
- Update electron to v29 and update other dependencies
- [calendar] fullDay events over several days now show the left days
from the first day on and 'today' on the last day
- Update layout of current weather indoor values

### Fixed

- Correct apibase of weathergov weatherprovider to match documentation
(#2926)
- Worked around several issues in the RRULE library that were causing
deleted calender events to still show, some
initial and recurring events to not show, and some event times to be off
an hour. (#3291)
- Skip changelog requirement when running tests for dependency updates
(#3320)
- Display precipitation probability when it is 0% instead of blank/empty
(#3345)
- [newsfeed] Suppress unsightly animation cases when there are 0 or 1
active news items (#3336)
- [newsfeed] Always compute the feed item URL using the same helper
function (#3336)
- Ignore all custom css files (#3359)
- [newsfeed] Fix newsfeed stall issue introduced by #3336 (#3361)
- Changed `log.debug` to `log.log` in `app.js` where logLevel is not set
because config is not loaded at this time (#3353)
- [calendar] deny fetch interval < 60000 and set 60000 in this case
(prevent fetch loop failed) (#3382)
- added message in case where config.js is missing the module.export
line PR #3383
- Fixed an issue where recurring events could extend past their
recurrence end date (#3393)
- Don't display any `npm WARN <....>` on install (#3399)
- Fixed move suncalc dependency to production from dev, as it is used by
clock module
- [compliments] Fix mirror not responding anymore when no compliments
are to be shown (#3385)

### Deleted

- Unneeded file headers (#3358)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Michael Teeuw <michael@xonaymedia.nl>
Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Karsten Hassel <hassel@gmx.de>
Co-authored-by: Ross Younger <crazyscot@gmail.com>
Co-authored-by: Bugsounet - Cédric <github@bugsounet.fr>
Co-authored-by: jkriegshauser <joshuakr@nvidia.com>
Co-authored-by: illimarkangur <116028111+illimarkangur@users.noreply.github.com>
Co-authored-by: sam detweiler <sdetweil@gmail.com>
Co-authored-by: vppencilsharpener <tim.pray@gmail.com>
Co-authored-by: Paranoid93 <6515818+Paranoid93@users.noreply.github.com>
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.

2 participants