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

Grouping Mechanism #83

Closed
wants to merge 17 commits into from
Closed

Conversation

CooperRedhat
Copy link
Collaborator

@CooperRedhat CooperRedhat commented Nov 17, 2021

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

  • Go here
  • Click step 3, hit next until you see it.

Issues/questions:

  • Clicking on the individual cards in expanded view does nothing.
    • Should the entire card act as a link to the corresponding quickstart? Just the icon or text?
  • I increased the font sizes from the design because they seemed very small in comparison to the newly increased font size in the rest of the drawer. Those can be changed back or to something else.
  • 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.
  • There is currently no way to store data from the proposed rating system. The thumbs up/down buttons do nothing.

@netlify
Copy link

netlify bot commented Nov 17, 2021

✔️ Deploy Preview for quickstarts ready!

🔨 Explore the source changes: 0590a55

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

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

@mceledonia
Copy link

mceledonia commented Nov 17, 2021

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:

  • Need some more padding above the learning path section, below the last line of content. ...--spacer--md would be a good place to start.
  • Try dropping the padding between cards to ...--spacer--sm, I had 4px in the mockups which was probably too tight but 16 feels a little wide.

@jgiardino
Copy link

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:

  • something replaces the buttons when clicked (even if only stored in app state)
  • showing this section is optional (i.e. a product like mine plans to actually track clicks on these)

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
Copy link

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
Copy link
Collaborator Author

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
Copy link

lboehling commented Nov 30, 2021

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
Copy link

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
Copy link
Collaborator Author

@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:

  • How can we allow for admins who are creating custom quick starts to group them, or create new groupings with a mix of custom quick starts and OpenShift quick starts? Could they add a new section to an already existing course?
  • There is also an openshift issue

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

packages/dev/src/LearningPathCatalog.tsx Outdated Show resolved Hide resolved
packages/module/src/controller/QuickStartConclusion.tsx Outdated Show resolved Hide resolved
packages/module/src/styles/style.scss Outdated Show resolved Hide resolved
packages/dev/src/App2.tsx Outdated Show resolved Hide resolved
@lboehling
Copy link

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
Copy link
Contributor

dlabaj commented Feb 16, 2024

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

@dlabaj dlabaj closed this Mar 7, 2024
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.

Grouping Mechanism to connect multiple quick starts and create learning paths
7 participants