Redesign series block #706

owi92 · 2023-02-13T15:24:50Z

This applies the suggested redesign of the series blocks by adding list and slider view options.
As with the other redesign PR, this is still missing the updated color scheme, which I suggest should be applied once every other part of the redesign is finished.

LukasKalbertodt

This looks really good! I think this makes Tobira feel quite a bit nicer. I like almost all of the things you did that were not part of Lisa's design or differ from Lisa's design. The slider view is particularly nice IMO. I totally didn't knew about these scroll-snap thingies. Works really well, also on mobile/with touchscreen. And everything seems to work very nicely with keyboard navigation as well!

However, as usual, I have comments :D Sorry for the large number of them, but all or almost all are very easy to fix, I think. Apart from the line comments, here are some general ones.

By using tab and enter (keyboard navigation), you can open both dropdowns at the same time.
The currently active option can still be clicked and is not shown as "active". Maybe we want to change that?
As you mentioned yourself: the shevron always points down, even if the menu is open. We might still want to fix that, but honestly I don't mind it that much. For what it's worth, in Lisa's design the icon also doesn't change for the open menu.
When opening one dropdown and then using the WAVE browser plugin, it shows an error.
In slider view: I can't scroll to the very start. Well I can but then it snaps to first element somehow which is sliiightly farther right than the start. Not a huge deal at all, but yeah.
I quite like the slider view on mobile -> why not make it possible to use it there? Only the list view makes no sense on mobile I think. Although... even then we could just put the description below the thumbnail and stuff. Actually... I think I'd like that. Then you can get rid of the whole "set state to 'grid' when window gets too small" code. Opinions?
The left/right buttons on Safari mobile are at the top instead of the center.

And one main one which does not need to be done in this PR: since those choices are completely local, they are reset on navigation and everything. I think that takes a lot of their use from them. In the future we likely want to persist those choices in local storage somehow (lotsa open questions) and we also want to give the same three options to the moderator designing the block. So that it can be a good default. But there are still lots of open questions as well. This can be done as an extra PR no problem.

frontend/src/ui/Blocks/Series.tsx

frontend/src/i18n/locales/de.yaml

frontend/src/ui/Breadcrumbs.tsx

frontend/src/ui/Blocks/Series.tsx

LukasKalbertodt · 2023-02-15T07:38:52Z

Another thing I thought of: on very small screens, the title has very little space (already now, but especially so when using my recommendtations of whitespace: nowrap and also showing the "view" setting on small screens). Maybe there should be some special case for that, e.g. set flex direction to column for screen smaller than 450 or sth like that?

This pulls the series title of blocks into the box and removes it's background for now, which should be transparent for the default design. It can of course still be configured to have another color.

owi92 · 2023-02-17T16:06:39Z

Ok, so I believe I managed to adress all your comments, but take that with a grain of salt. Unfortunately I forgot to do this a little more atomically, so it might again be a little frustrating to review - sorry for that! And if that is a larger issue, I could of course go back and make smaller commits out of these changes. Also, when pushing the rebased status a little earlier today, I forgot that I already did an amend on the first commit of this - but that is only the reversion of the breadcrumb changes.

Note that this is still missing the ability to use the menus with arrow keys.

frontend/src/i18n/locales/en.yaml

frontend/src/ui/Blocks/Series.tsx

LukasKalbertodt · 2023-02-20T10:43:38Z

frontend/src/ui/Blocks/Series.tsx

+                        [`@media not all and (max-width: ${VIDEO_GRID_BREAKPOINT}px)`]: {
+                            marginLeft: "auto",
+                        },
+                        [`@media (max-width: ${VIDEO_GRID_BREAKPOINT}px)`]: {
+                            width: "100%",
+                            justifyContent: "space-between",
+                            paddingLeft: 11,
+                            paddingRight: 11,
+                        },


Is it intended that the two buttons have space between them? I would think it looks better if they are both on the right.

So I would remove all of these lines. And add justifyContent: flex-end but without media query. The replace the margin-left here, the parent div should have justify-content: space-between. Unless I'm missing something that works better with less CSS and it looks better IMO.

The only "problem" this is that if the title is missing, the buttons are on the left again. I think I would solve that by just removing title &&. So that the <div><h2 /></div> (or even just <h2 /> as mentioned above) is always rendered, but possibly with nothing inside.

Yeah I did space-between because I found the situation depicted below to look a little awkward when the title is short, but I suppose it's not that bad. I could also just use a lower breakpoint for flex-direction: column, or align (or rather "justify") the buttons with the heading on the left on smaller screens (second screenshot). Let me know what you prefer.

I believe that justify-content: flex-end does not work with align-self: flex-start (which I use to align the buttons vertically with the first line of multiline headings), but we can use margin-left: auto instead of justify-content to keep the buttons on the right.

Ah mh, thats fair, it does look weird for small titles :/
What about using flex-wrap: wrap instead of flex-direction: column? Then the second line is only created when there is actually not enough space. Specific suggestion:

Remove all 5 CSS properties that this review comment is attached to.

Add margin-left: auto to the button container (for all screen sizes)

For width < breakpoint, add flex-wrap: wrap for the outer container (and remove flex-direction: column)

For any given title, there is still a screen width where it looks weird (since it juuust wrapped around), but thats better than the previous idea. What do you think?

Sure, that works for me!

I like how it works now I think. The justifyContent: "space-between" on the parent div is not even required anymore.

frontend/src/ui/Blocks/Series.tsx

LukasKalbertodt

Close to being done! Just a few more things.

frontend/src/ui/Blocks/Series.tsx

owi92 · 2023-02-20T17:23:33Z

One last thing that still bugs me are the different margins between the block-header and the upcoming events grid (and to a lesser extent also the left/right margin difference of the header). We could adjust these but then the videos from the upcoming events wouldn't align with the videos from the regular grid unless the upcoming events had less padding. 😖 But I guess that doesn't need to be done or even decided in this PR.

LukasKalbertodt

Almost ready! Two more thoughts:

If there are no videos in the series, we already show a special "no events in series" or sth. In that case we should probably also not show these sorting/view buttons as they don't do anything. If that's easy to do, maybe include it in this PR still.
You added the arrow tips now and ... I think without looks better actually. At least in this case. If we go with arrows, there should definitely not be distance={0}. The tips shouldn't touch the button. But ye, I would probably go without. But no hard opinion. What do you think?

Regarding the margins of "upcoming events grid"... yeah. I wondered about that too when I reviewed the PR where you added that. I don't mind the current state of it. But ye, one could also tweak that still.

frontend/src/ui/Blocks/Series.tsx

owi92 · 2023-02-20T19:57:38Z

Ok, so there is a simple way to check if the children of the SeriesBlockContainer contain any actual events, but it feels kinda hacky... I included it for now, but I'm thinking there must be a nicer way that I am missing.
(Edit: To see this in action check out https://pr706.tobira.opencast.org/conferences/2009/systemsx-ch for instance... I always forget which series are eventless;)

Regarding the arrow tips: I included them in the previous iteration because I was curious about your opinion. But I also prefer the menus without them, so I removed them again.

LukasKalbertodt · 2023-02-21T09:53:20Z

Yeah, it's nicer without arrow tips.
And I agree, the way you check whether there are events is a bit too hacky for my taste. I would just add a hasEvents: boolean prop to the component which can be passed down directly from the parent as that parent has access to the events array.
Regarding the re-indentation you did: in all those case both options are totally fine to me. So yeah, just keep it as it is now.
One more thing I noticed: can you add lines={3} to the <SmallDescription>? The default is 2, but 3 fit no problem in the desktop view. So we should show that additional information.

frontend/src/ui/Blocks/Series.tsx

owi92 · 2023-02-21T11:17:45Z

frontend/src/ui/Blocks/Series.tsx

@@ -162,7 +189,7 @@ const ReadySeriesBlock: React.FC<ReadyProps> = ({
            {...{ basePath, event }}
        />);

-    const eventsUI = events.length === 0
+    const eventsUI = !eventsNotEmpty


Added this check because before we would show the no-events text even if there were upcoming events present

This adds a list and slider view option to the series blocks.

LukasKalbertodt

Wonderful! 🎉

github-actions bot temporarily deployed to test-deployment-pr706 February 13, 2023 15:30 Destroyed

owi92 force-pushed the redesign-series-block branch from c5cad4c to b5b7724 Compare February 13, 2023 15:35

github-actions bot temporarily deployed to test-deployment-pr706 February 13, 2023 15:40 Destroyed

LukasKalbertodt requested changes Feb 14, 2023

View reviewed changes

Simplify display of series title.

fec9f82

This pulls the series title of blocks into the box and removes it's background for now, which should be transparent for the default design. It can of course still be configured to have another color.

owi92 force-pushed the redesign-series-block branch from b5b7724 to 6aaa88e Compare February 17, 2023 10:48

github-actions bot temporarily deployed to test-deployment-pr706 February 17, 2023 10:52 Destroyed

owi92 force-pushed the redesign-series-block branch from 6aaa88e to 938a665 Compare February 17, 2023 15:36

github-actions bot temporarily deployed to test-deployment-pr706 February 17, 2023 15:40 Destroyed

owi92 force-pushed the redesign-series-block branch 2 times, most recently from df2838e to 08334af Compare February 17, 2023 15:52

github-actions bot temporarily deployed to test-deployment-pr706 February 17, 2023 15:57 Destroyed

LukasKalbertodt reviewed Feb 20, 2023

View reviewed changes

LukasKalbertodt requested changes Feb 20, 2023

View reviewed changes

owi92 force-pushed the redesign-series-block branch from 08334af to 3d03d36 Compare February 20, 2023 16:56

github-actions bot temporarily deployed to test-deployment-pr706 February 20, 2023 17:01 Destroyed

LukasKalbertodt requested changes Feb 20, 2023

View reviewed changes

frontend/src/ui/Blocks/Series.tsx Outdated Show resolved Hide resolved

owi92 force-pushed the redesign-series-block branch from 3d03d36 to 0710204 Compare February 20, 2023 19:48

github-actions bot temporarily deployed to test-deployment-pr706 February 20, 2023 19:52 Destroyed

owi92 force-pushed the redesign-series-block branch from 0710204 to ac5b59c Compare February 21, 2023 11:12

owi92 commented Feb 21, 2023

View reviewed changes

frontend/src/ui/Blocks/Series.tsx Outdated Show resolved Hide resolved

github-actions bot temporarily deployed to test-deployment-pr706 February 21, 2023 11:17 Destroyed

owi92 commented Feb 21, 2023

View reviewed changes

Apply series block redesign

5b5743e

This adds a list and slider view option to the series blocks.

owi92 force-pushed the redesign-series-block branch from ac5b59c to 5b5743e Compare February 21, 2023 12:43

github-actions bot temporarily deployed to test-deployment-pr706 February 21, 2023 12:47 Destroyed

LukasKalbertodt approved these changes Feb 21, 2023

View reviewed changes

LukasKalbertodt merged commit 5bc27a2 into elan-ev:master Feb 21, 2023

LukasKalbertodt added the changelog:user User facing changes label May 15, 2023

owi92 deleted the redesign-series-block branch March 4, 2024 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign series block #706

Redesign series block #706

owi92 commented Feb 13, 2023

LukasKalbertodt left a comment

LukasKalbertodt commented Feb 15, 2023

owi92 commented Feb 17, 2023

LukasKalbertodt Feb 20, 2023

owi92 Feb 20, 2023

LukasKalbertodt Feb 20, 2023

owi92 Feb 20, 2023

LukasKalbertodt Feb 20, 2023

LukasKalbertodt left a comment

owi92 commented Feb 20, 2023

LukasKalbertodt left a comment

owi92 commented Feb 20, 2023 •

edited

LukasKalbertodt commented Feb 21, 2023

owi92 Feb 21, 2023

LukasKalbertodt left a comment

Redesign series block #706

Redesign series block #706

Conversation

owi92 commented Feb 13, 2023

LukasKalbertodt left a comment

Choose a reason for hiding this comment

LukasKalbertodt commented Feb 15, 2023

owi92 commented Feb 17, 2023

LukasKalbertodt Feb 20, 2023

Choose a reason for hiding this comment

owi92 Feb 20, 2023

Choose a reason for hiding this comment

LukasKalbertodt Feb 20, 2023

Choose a reason for hiding this comment

owi92 Feb 20, 2023

Choose a reason for hiding this comment

LukasKalbertodt Feb 20, 2023

Choose a reason for hiding this comment

LukasKalbertodt left a comment

Choose a reason for hiding this comment

owi92 commented Feb 20, 2023

LukasKalbertodt left a comment

Choose a reason for hiding this comment

owi92 commented Feb 20, 2023 • edited

LukasKalbertodt commented Feb 21, 2023

owi92 Feb 21, 2023

Choose a reason for hiding this comment

LukasKalbertodt left a comment

Choose a reason for hiding this comment

owi92 commented Feb 20, 2023 •

edited