Add news announcement carousel to dashboard #9617
Conversation
| return ( | ||
| <div className={`${bn}__indicators`}> | ||
| {this.props.announcements.map((_, index) => ( | ||
| <div |
There was a problem hiding this comment.
these should probably be clickable
There was a problem hiding this comment.
they're so small (6px max height) that it's kind of annoying to try to click them. I can't tell from the design if they were intended to be buttons...
There was a problem hiding this comment.
I don't think it's too bad as long the button expands on hover
There was a problem hiding this comment.
added -- I was thinking that if the button still feels too thin, the hoverable/clickable height could be increased while keeping the display as-is. but I'll wait for your input
|
I just remembered, should this be shared with in-client banner system? The actual image will be different but I imagine there should be some overlaps in the content itself (if not exactly the same). |
|
the client banner can only display one at a time (but i do believe i've heard they can be set to automatically rotate) while this website carousel can display multiple. the intention with the linked issue was that they would have the same type of content, yes (and as mentioned in op, eventually displayed in lazer too) |
|
So the client banner currently isn't stored anywhere. The new table here should add |
Hide overflow on the top element rather than the announcements container, so now the container can be transformed by CSS rather than scrolled through. And clean up many other things that resulted from not needing to track element widths and such.
And add transition to height
|
okay, a lot of updates here. the main react component is basically rewritten now to use only CSS for the positions and animations, which resolved a lot of your reviews just by being able to get rid of all that ref/observer/etc stuff (ee2dd65) I also made it support that "infinite scrolling" feel since it wasn't actually very difficult after rewriting it (tl;dr clone the announcements as needed to make it appear as if you can scroll to one side forever; delete unnecessary clones when it stops animating) (dfd92c8)
yep that's the goal as mentioned in OP, it should be able to work for web, stable, and also lazer whenever it gets a similar feature. just website for this PR though. once this PR is in, I'll get in touch w/ ephemeral or whoever is managing the main menu banners currently, to figure out how this system could be shared best |
So it won't be confusing naming when there are multiple kinds of buttons
database/migrations/2023_08_07_100000_create_news_announcements.php
Outdated
Show resolved
Hide resolved
| {/* | ||
| Render the announcements, including clones before and after to help | ||
| create the illusion of an infinitely scrolling container | ||
| */} |
There was a problem hiding this comment.
where's the clone? the range below only generates exactly the length of announcements.
From what I can tell the illusion currently happens because the clone is generated exactly before the animation starts?
(oh and unless you go furious with clicking, index - 2, index + 3 range also works)
There was a problem hiding this comment.
hopefully clarified this explanation
for simplification to index - 2, index + 3 or something similar -- i mean it's pretty straightforward to track min/max index so i think this is fine as-is, even if it only practically improves an edge case
|
@nanaya there was some discussion about this that amounted to nobody being sure if this should share the same data as the in-game banners, because those often link to news on the front page, and it would be kind of redundant. ppy/osu#27680 (comment) (and then more on discord but there was no real conclusion) personally I don't find sharing data to be problematic as long as whoever controls it is mindful about not literally duplicating content from news banners. but whoever those ppl are should probably be the ones deciding how this works I'll respond to the previous review in the meantime |
|
That new design looks quite bad for the caroussel, main menu banners are not really designed to float in the middle of an area |
most of the recent banners would only require minor adjustments if any at all |
resources/css/bem/menu-image.less
Outdated
|
|
||
| .menu-image { | ||
| height: 100%; | ||
| left: calc(100% * var(--index, 0)); |
There was a problem hiding this comment.
the variable should be defined otherwise it may end up using unrelated variable from its parents
resources/css/bem/menu-images.less
Outdated
| &::before { | ||
| .fas(); | ||
| content: var(--icon); | ||
| text-shadow: 0 1px 2px rgba(0, 0, 0, 0.25); |
There was a problem hiding this comment.
seems fine with default text shadow
| <div | ||
| className={bn} | ||
| style={{ '--index': index } as React.CSSProperties} | ||
| > |
There was a problem hiding this comment.
this div can just be the anchor instead?
| }; | ||
|
|
||
| private readonly handleIndicatorClick = (event: React.MouseEvent<HTMLButtonElement>) => { | ||
| // Increment the index by the visible difference between the selected and |
There was a problem hiding this comment.
is it even possible to trigger this when the index is outside normal range... (as opposed to just setIndex(dataset.index))
| @observable private maxIndex = this.length - 1; | ||
| @observable private minIndex = 0; |
There was a problem hiding this comment.
these can just be getters?
private get maxIndex() {
return Math.max(this.length - 1, this.index);
}
private get minIndex() {
return Math.min(0, this.index);
}
...or maybe even inlined at the only place they're used, although that'd be a bit long
app/Libraries/MenuContent.php
Outdated
| $images = self::parse(self::fetch()); | ||
| $now = Carbon::now(); | ||
|
|
||
| return array_values(array_filter($images, function ($image) use ($now) { |
There was a problem hiding this comment.
can use fn ($image) => ...
| overflow-x: hidden; | ||
| position: relative; | ||
|
|
||
| &__arrow { |
There was a problem hiding this comment.
maybe only show this when hovering the area
|
@cl8n any plan to update this? Otherwise I can do it instead. |
|
not soon so yeah you can take it over if it's a priority |
The linter doesn't quite render less variable access right so rework it for now.
|
Is this missing a push 🤔 |
|
if only I pushed it to the correct repository/branch 👀 |
changes from the design on figma:
i wasn't sure how it was supposed to animate so I made it slide horizontal. if you aren't interacting with it, it'll automatically rotate every 6 seconds (arbitrary), and otherwise you can control it yourself with the buttons on the side
resolves #9614