plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
426 stars 576 forks source link

Add Summary Box tile (Proxy) #573

Closed pnicolli closed 1 year ago

pnicolli commented 5 years ago

Addresses #288 Functionality seems ok. One info I didn't ask in Tokyo is: how should it look like? I have used a Card from semantic-ui with a mini image miniature for now, as an example, but I guess this tile should have a better default, like showing the image at full width, I don't know... is there any written info I missed about that? I couldn't look up other UI components from Semantic right now, because I developed the last part of it on the plane 😄 Any feedback or suggestions are welcome.

coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 2699


Totals Coverage Status
Change from base Build 2683: 0.0%
Covered Lines:
Relevant Lines: 0

💛 - Coveralls
pnicolli commented 5 years ago

If #575 is merged, I should remove the custom actions and reducers added here and use subrequests for fetching things from the backend. Just leaving this note here for future reference for myself and/or others, while this PR is WIP.

pnicolli commented 5 years ago

@robgietema Refactored in order to use new subrequests instead of creating new actions and reducers.

tisto commented 5 years ago

@pnicolli if I recall correctly we agreed that the summary tile should always show the title, description, and leadimage. @sneridagh did we get mocks from @albertcasado regarding this in Tokyo?

sneridagh commented 5 years ago

@tisto not really, only the "wireframes" on the whiteboard. I guess that with something basic will be ok until next time Albert can provide something more accurate.

sneridagh commented 5 years ago

@pnicolli LGTM! I'm currenty making a wish list for @albertcasado about Tiles icons :) When we have the correct one, we can merge it.

@albertcasado we also need a default "card" look & feel for Pastanaga UI.

sneridagh commented 5 years ago

@pnicolli Finally had the chance to take a closer look (running actually locally) and I think we need to come out with a layout that seems like an item in the Summary view in Plone, then a link to the real thing. What do you think? I think this is what we talked in Tokyo, it might be something like this mock:

group

/cc @pnicolli @tisto @robgietema @polyester what do you think?

@polyester or did we want the full content there? If we want that, we might want to add a "Full proxy tile".

polyester commented 5 years ago

@sneridagh yep, that's what I had in mind, just the Title, LeadImage, Summary. Certainly not the full content.

From an a11y standpoint, it is desirable that links to the actual content are 'meaningful', so no "Read more" a million time, it is better to link Title and LeadImage (or, if you prefer, also the summary as link) with a link that mentions the title again.

And, from a11y standpoint, it is desirable that a link is recognizable as link, so the Title would have to show some indication (underline, ...) that it is also a link. But that's for the CSS wizards to make look nice yet still accessible...

pnicolli commented 5 years ago

I agree with you @sneridagh and @polyester. I will update this according to the mock asap, so we can see if it fits with the rest of the default volto styles.

I see two options here for linking the original content:

  1. Linking title and image, with a slight transparent overlay when hovering the image and standard link styles on the title.
  2. Linking the whole box that contains the image/title/summary. That would mean linking the whole area in the mock @sneridagh pasted here in the previous comment. This would mean adding proper styles to highlight the box, of course, otherwise it would be a pain from the UX pov.

I would go for option 1, but what do you think?

polyester commented 5 years ago

@pnicolli option 1. That's also preferable from an a11y standpoint, as it signals there is a link on the link title for people who can't hover

pnicolli commented 5 years ago

@sneridagh Updated with the new tile api, new tile style, moved changelog entry to next version, updated snapshot test.

sneridagh commented 5 years ago

@pnicolli Looks great! Comments:

unnamed (1)

unnamed

(I still have to modify the existing tiles toolbars to match the X -> schema), a reusable component that has it all already is in the makings.

And the most important, we need a object tree navigator that should be also unified thorough all the UI. We are still waiting for an Albert's proposal, but should unfold under the toolbar most probably and show a navigation. I expect to work on this during Sorrento Sprint next week.

I will also work in Sorrento in documentation on how to develop tiles, since we have nothing written yet (sorry!). Any help on the toolbar/navigator/etc that I mentioned will be welcomed!

pnicolli commented 5 years ago

@sneridagh updated icon and copiedover the default keydown behavior from other tiles. Setting this keyboard behavior disables the default semantic-ui Dropdown keyboard behavior, which was used for content selection in the toolbar, but probably won't be a problem. Still, it needs to be tested and I agree that a common search toolbar with a common behavior is what we should achieve.

In your toolbar proposal I see an icon to the left of the input field, which I suppose is the specific tile icon, right? Then i see an X right after the input, which I assume clears the input, am I correct? I don't immediately get what the arrow does in this common toolbar. Anyway, I think you will be better working on the toolbar in a different PR. I won't be in Sorrento but once the work on that has started I will be happy to help on that, I like the idea of creating a common search toolbar component for these tiles.

If you have any other comments on this, feel free to add them, I have some time to polish this PR this week :)

pnicolli commented 5 years ago

Regarding tile names and variations, would you prefer to rename this from Summary to Proxy before merging, or would you rather wait for more discussions about variations in the future?

sneridagh commented 5 years ago

Great! Thanks @pnicolli !! Yeah, you are right, the focus should also be tweaked to make the tab and arrows still work inside the toolbar. In the Hero tile I managed to kind of make it work, but we must work on it a little bit to improve it.

Indeed, the toolbar should make the user aware of what kind of element is going to be inserted (e.g an image, or a link) then expose an X to clear the field, and a -> for submit it (load the image, link, map, etc), all this maintaining accessibility (which it's not easy).

Yes, I have an initial implementation of the common toolbar locally, I will try to push the WIP work this week, to continue work on it in Sorrento. I have also an initial idea of how the object tree should look like from Albert.

Regarding the tile variation, yes, I would rename it befor merging, even also implement the variation probably in Sorrento (since it's not hard), what do you think it would be the better name? Proxy/Content tile is fine with me. /cc @robgietema @tisto @polyester

I will take a look at your amendments later today, thanks for the effort!

sneridagh commented 4 years ago

@pnicolli what's the status here? It seems super outdated, right? Should we leave it to revisit it in the next sprints?

pnicolli commented 4 years ago

@sneridagh I think we might have talked about it during the conf sprint, but I'm not sure, maybe I just wanted to but couldn't... Anyway, I think what I wanted to discuss is that maybe it makes sense to integrate something like the Teaser block you showed us at the conf. If that's possible, maybe this could be somewhat replaced by that. Otherwise, this was actually ready to go the last time I worked on it, but it needed a proper name.

There was a discussion about having two versions of it, or two possible configurations, in order to choose whether to show just image, title and description, or to show the full content of the proxied item.

tisto commented 4 years ago

@pnicolli @sneridagh we should schedule another Volto hangout to discuss those issues, what do you think? Early next week?

pnicolli commented 4 years ago

@tisto yeah, we have a couple things we would like to discuss, it would make sense. I am available up to wednesday, then I won't be at work until next tuesday.

sneridagh commented 2 years ago

@pnicolli I think that this is long superseded by a listing block with the Summary view, using a limit of 1 :) We are thinking indeed into adding the Teaser to core or release it as an add-on too.

What do you think?

tiberiuichim commented 2 years ago

+1 on teaser in core

pnicolli commented 2 years ago

It's almost incredible how, after years of sleeping at the bottom of the PR pages, we at Redturtle were also bringing out this old PR today 😂 I agree we should have a Teaser block in core as well. It's way better UX for a user that wants to highlight another content, instead of adding and configuring a listing block properly to make this work. Currently for our biggest theme, for example, we have a custom Teaser block implementation to be used in the homepage, which shares the UI with a listing variation in order to have both options available.

sneridagh commented 2 years ago

Really? 😂😂

Me, today: "Let's try to make some house clean up, let's start from the bottom"

Also me, after commenting in the last one: "Meh, too many not so clear ones" then leaving it for later. 😁

sneridagh commented 2 years ago

Superseded by: https://github.com/plone/volto/pull/3180

sneridagh commented 2 years ago

@pnicolli do you think we can "port" any idea to there? Leaving the PR open for now.

sneridagh commented 1 year ago

superseded by: #2779