patternfly / patternfly-quickstarts

Repository containing the PatternFly Quick Start extension code
MIT License
15 stars 39 forks source link

Grouping Mechanism #83

Closed CooperRedhat closed 8 months ago

CooperRedhat commented 2 years ago

Closes #37

image

Majority of the design is implemented. I thought I'd get this out there to allow design review to begin.

Best way to preview the new feature

Issues/questions:

netlify[bot] commented 2 years ago

✔️ Deploy Preview for quickstarts ready!

🔨 Explore the source changes: 0590a557ed450fcf4ed62f2901e353bc566813bd

🔍 Inspect the deploy log: https://app.netlify.com/sites/quickstarts/deploys/61d5d12b1badf00007aa6b41

😎 Browse the preview: https://deploy-preview-83--quickstarts.netlify.app/

mceledonia commented 2 years ago

This is looking great. I think the new text sizes work well!

For the cards behavior, I am leaning towards the whole card being the target and linking to that quickstart. In that case we would want a hover state on the cards which would just be increasing the shadow variable up a step, try from $pf-global--BoxShadow--sm hover -> --md or --lg if md doesn't feel like enough.

I would like to hear what others think about that though, any thoughts @CooperRedhat @jessiehuff @bmignano @jgiardino

Other notes:

jgiardino commented 2 years ago

For the cards behavior, I am leaning towards the whole card being the target and linking to that quickstart. In that case we would want a hover state on the cards which would just be increasing the shadow variable up a step, try from $pf-global--BoxShadow--sm hover -> --md or --lg if md doesn't feel like enough.

This approach sounds good to me!

In the expanded view, the first card in the list is the current QS. It seems like it there should be some indication of that.

Looking at this the first time, I did get lost when expanding/collapsing the section. I usually associate expand/collapse with show all vs show nothing. But here it's more like the quick starts are filtered to show the recommended next quick start by default, with an option to show all quick starts in the path. Would it work to replace the ▶️ 🔽 buttons with a Show all and Show recommended button? Or maybe a toggle group like the example here would work?: https://www.patternfly.org/v4/guidelines/filters#toggle-group

But regarding @CooperRedhat's question, I was wondering if the recommended quick start that always displays should have more visual prominence and more visual consistency between the two views so that it's easy to pick out in the full list quick starts. I think this could help, and maybe this is a way of addressing @CooperRedhat's question above (i.e. show me where I'm at by highlighting where to go next). 🤷

There is currently no way to store data from the proposed rating system. The thumbs up/down buttons do nothing.

In our product, we use pendo for tracking user clicks throughout the app. So that's something that we could use here. It would require some ability to uniquely identify each button. Pendo supports basic CSS selectors, so a class on each button, plus some way to identify in a parent selector which quick start they are rating would be what we need to measure these clicks.

But even so, the point about the thumbs up/down buttons doing nothing is still valid. It would be good to have an idea about what the user should see next (i.e. "Thanks for your feedback!") and also the expectation that most likely these buttons will come back as if they were never clicked when the user refreshes the page.

My take on this is that it should not be included unless:

One final comment on the rating section - Looking at it for the first time, it took me some time to realize that this section isn't really a peer to the other cards above it because it has equal visual presentation. Just wanted to mention it here in case others saw that too. But also, that can be a whole separate discussion from this PR. (actually, there are probably several comments from me above that could be handled outside this PR 🙃 )

bmignano commented 2 years ago

I agree with everything that was already said! I might add that we may want to add a little padding beneath the last card in the learning path section—it looks to be decently smaller than the top and side padding so it feels uneven.

I think the other thing we may have briefly discussed before too is pulling the rating bit out of the learning path section to just below the "Congratulations!..." message. This way it's much more clear that you are rating the quick start you just finished as opposed to the entire learning path. Especially with the text "You're all finished!" it makes me think it's for the whole path until I read the next sentence.

I also think the expand / collapse interaction is a little odd. Maybe if we take out the rating from that section then it will make more sense but agree with Jenn that usually I expect carets to show or hide all items.

A smaller nit, I'm not sure we've ever socialized or otherwise promoted "QS" as a shorthand for quick starts. We may just want to write out "Recommended next quick start".

CooperRedhat commented 2 years ago

A smaller nit, I'm not sure we've ever socialized or otherwise promoted "QS" as a shorthand for quick starts. We may just want to write out "Recommended next quick start".

Nice catch Brie, that was not intended. The design uses "Recommended next course", "QS" is a shorthand I was using in code and probably got caught in a find-replace action.

lboehling commented 2 years ago

I think this looks really great @CooperRedhat!

Chiming in on the design discussion:

I agree with all of the suggestions/comments above, especially regarding the expand/collapse interaction feeling a bit off. I think that if we pull the rating card into its own section (per @bmignano's suggestion), that gives a bit more affordance to put a "show all/show less" button directly below the recommended card(s), instead of using the ▶️ 🔽 buttons in the header.

I mocked up this suggestion, along with the others mentioned above (adding more space above the learning path section, decreasing padding between cards, adding a visual indicator for the current QS card, keeping the indicator for suggested QS in the expanded view, and moving the rating card above/out of the learning path section), and would love to know what you all think about these edits. @mceledonia @jgiardino @bmignano @jessiehuff

image

Also, I have a quick question -- is the intention of the learning path to replace and give more context to the QS links between the "congratulations!..." message and the learning path section, or are both of those sections going to still be included? Having both seems a bit repetitive to me right now.

jgiardino commented 2 years ago

You updates look great, @lboehling!

Also, I have a quick question -- is the intention of the learning path to replace and give more context to the QS links between the "congratulations!..." message and the learning path section, or are both of those sections going to still be included? Having both seems a bit repetitive to me right now.

This is a really great question. I'll offer my thoughts on this, but ultimately defer to anyone who has actual knowledge on requirements and vision (I don't think I do 🙃). I see these sections as an either/or. Either you started a learning path that is a specific sequence of quick starts, or you stumbled across a quick start in the wild that you completed. In the latter case, there might be several recommendations that we could make. E.g. after a user creates a Kafka, there are several things that we could suggest they look at next.

If that is an accurate assessment of the requirements/vision, I could definitely see how your design could be applied to the "congratulations!..." message and suggested quick starts.

CooperRedhat commented 2 years ago

@lboehling, thanks for the mock up to get all the suggestions in one place, that helps a lot!. I really like the way it looks, and I think the show more/less is more intuitive than the arrow expander.

I agree that having the suggested quick starts and a learning path showing is repetitive. I wouldn't imagine that a quick start would include both. I created a custom view in the demo app, a Learning Path Catalog, which is a bit closer to what I'm imagining a real use case to be. Here's the preview link.

As far as requirements, here is a relevant blurb from the exploration doc:

It seems like there is a consensus on @lboehling's mock, so I'll make those changes.

lboehling commented 2 years ago

This looks awesome @CooperRedhat!

two small adjustments--Could you add a bit more padding above the learning path title and could you reduce the padding around the quickstart cards to 16px? I added those two adjustments to the image on the right. Thank you!

Screen Shot 2021-12-17 at 12 39 53 PM Screen Shot 2021-12-17 at 12 39 53 PM

dlabaj commented 8 months ago

@jgiardino @jschuler Do we still need this? Can we close this PR?