Skip to content

Updates "Section Settings" Side panel UI #13239

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

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

AllanOXDi
Copy link
Member

@AllanOXDi AllanOXDi commented Mar 21, 2025

Summary

This PR updates the section title in the edit section UI

Closes #13107

After

Screenshot 2025-03-21 at 16 46 53

Before

Screenshot 2025-03-21 at 16 48 06

References

#13107

Reviewer guidance

  1. See https://www.figma.com/design/lVJt5ukOrxS9qay0rXy7GC/0.18-Coach-updates?node-id=107-20607&p=f&t=VBq8IMW8iRQB1p7u-0

  2. Navigate to Create quiz

  3. Click on the Options button

  4. Select the edit section

@AllanOXDi AllanOXDi requested a review from nucleogenesis March 21, 2025 13:53
@github-actions github-actions bot added APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend labels Mar 21, 2025
Copy link
Contributor

github-actions bot commented Mar 21, 2025

@rtibbles rtibbles added the P1 - important Priority: High impact on UX label Mar 24, 2025
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I note a few differences to the Figma but I think they're likely not binding requirements here - but noting in case @jtamiace thinks otherwise.

  • The Figma doesn't show a border-line beneath the side panel title
  • The Y-padding for the buttons footer is a bit taller than is currently implemented (FWIW I think the current implementation looks good as-is but also I played part in making it so I may be biased 😅)

@@ -311,7 +306,6 @@
maxQuestionsLabel,
// i18n
displaySectionTitle,
sectionSettings$,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a quick grep - looks like this string is no longer used anywhere with it being removed from here so we could probably remove it from the quiz strings file.

@jtamiace
Copy link
Member

Agreed, there shouldn't be a border beneath the title section, and there also shouldn't be a border on the bottom bar. I think the border under the title section is currently implemented in different side panels across the app and it'd be good to remove that at some point

Copy link
Member

@akolson akolson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes seem to make sense to me. I'll leave the final approval to @nucleogenesis @marcellamaki whenever they are ready. Thanks @AllanOXDi

@AllanOXDi
Copy link
Member Author

HI @jtamiace does this change looks good to you?

Screenshot 2025-04-01 at 22 07 29

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @AllanOXDi - this is definitely a good start, but there are a few other changes in terms of spacing in the Figma that I think would be good to include while we're here. At the minimum, the title should be aligned with the content to the left, as seen in the figma. Currently, it's a bit off.

Screenshot 2025-04-10 at 1 38 25 PM

Also in the figma, the spacing just as a bit more "breathing room". You can see that the spacing is just a little bit larger that what we currently see in the UI. This isn't essential, but it's a perfect opportunity to practice "pixel-perfect" (or close to it) css refinement.

@@ -461,7 +455,6 @@
padding: 1em;
margin-top: 1em;
background-color: #ffffff;
border-top: 1px solid black;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we don't want the solid black border here, the figma shows a dropshadow, so we should replace with that, rather than just remove the border

Screenshot 2025-04-10 at 2 04 56 PM

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HI @AllanOXDi - thanks for making the spacing updates on the side. Visually it looks better in the browser.

One blocking problem is that this change is causing a regression with the other section side panel which edits the section order.

Screenshot 2025-05-07 at 8 49 56 AM

What we need to do is:

  • keep a consistent design pattern for the title
  • not have multiple headers
  • make sure that in the other side panel, the user knows they are editing the order

I think there are several ways to accomplish this - maybe you can suggest an option that seems technically feasible and show Jessica what it looks like for her 👍

One non-blocking request is that you revisit some of the other spacing/layout updates in the figma, such as the space between elements, etc. that I mentioned. If you're not sure exactly what's in scope here after reviewing the Figma, please ask and we can look at it together.

@marcellamaki
Copy link
Member

As a secondary thought - if you run into challenges with this (updating the CSS styles, the title/header patterns, etc.), this is really useful information for our side panel refactoring work! You can share that as part of our conversations

Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AllanOXDi! I just left some minor observations, mostly just nitpicks, but the i18n one I think its something that we must solve before merging this pr, please feel free to ask any question if needed :).

v-if="route.name === PageNames.QUIZ_SECTION_ORDER"
class="sidepanel-title"
>
{{ coreString('editAction') }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting that we are changing from "Edit - Section order" to "Edit section order". Is this an intentional change? If we want the "Edit section order" string, I dont think that doing coreString('editAction') + sectionOrderLabel$().toLowerCase() would be the best way to achieve it, since many languages can have different grammar and have, lets say, the verb at the end of the sentence. So my best guess would be to return to the "Edit - Section order" if we dont want to introduce any new strings for this PR (since its targeted for the planned patch 2).

@@ -130,6 +130,11 @@
required: false,
default: false,
},
addBottomBorder: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marcellamaki another discrepancy in the side panels 😆. In the 0.18 designs we didn't have a separator line below the header in any of the frames in figma. But in the implementation of this component, we do have this line. I think for consistency we should have or should not have this line for all the side panels of at least the same section. So either option, or we show this line for the SectionSidePanel component, or we also remove this line from the resource selection side panel (or we remove this line within this component for all side panels), what do you think?

Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a final thought 😄, after this, this looks good to me! Will defer the final approve and merge to @nucleogenesis or @marcellamaki.

class="sidepanel-title"
>
{{ editAction$() }} -
{{ sectionOrderLabel$().toLowerCase() }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can to sectionOrderLabel$() instead of sectionOrderLabel$().toLowerCase() now that we have that dash again.

@AlexVelezLl AlexVelezLl self-requested a review June 13, 2025 17:08
@AllanOXDi AllanOXDi force-pushed the update-section-settings branch from 8f68972 to d71c39e Compare June 16, 2025 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend P1 - important Priority: High impact on UX SIZE: small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update "Section Settings" Side panel UI
7 participants