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

Remove bottom margin for gr-has-no-nested-questions #34032

Merged
merged 1 commit into from
Jan 26, 2024
Merged

Conversation

nospame
Copy link
Contributor

@nospame nospame commented Jan 25, 2024

Product Description

Fixes a bug introduced with a recent deploy where nested groups with no visible questions add extra vertical spacing.

Before:
image

After:
image

Technical Summary

USH-4154

Feature Flag

Safety Assurance

Safety story

The smallest CSS change to margin, very targeted.

Automated test coverage

n/a

QA Plan

No QA. Support/Delivery to verify on staging.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@nospame nospame marked this pull request as ready for review January 25, 2024 21:47
@nospame nospame requested a review from orangejenny as a code owner January 25, 2024 21:47
@nospame nospame added the product/all-users-all-environments Change impacts all users on all environments label Jan 25, 2024
Copy link
Contributor

@orangejenny orangejenny left a comment

Choose a reason for hiding this comment

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

I wonder if there's any way we could make it more apparent that when updating group styling, you might also need to update these has-no-questions classes. Any bright ideas?

@nospame
Copy link
Contributor Author

nospame commented Jan 25, 2024

I wonder if there's any way we could make it more apparent that when updating group styling, you might also need to update these has-no-questions classes. Any bright ideas?

@orangejenny My first thought is probably more commenting, but in this case I think changes were made to code generating the elements that happen to get those classes, not even really to the elements or classes themselves, so it would have been easy to miss anyway.

QA honestly seems like the best chance to catch unexpected UI things like this that result from particular configs. I know that I personally would not have considered the case of a group-within-a-group where neither group has any visible questions.

@nospame nospame merged commit 9ab2e3e into master Jan 26, 2024
12 checks passed
@nospame nospame deleted the em/ush-4154 branch January 26, 2024 19:45
@orangejenny
Copy link
Contributor

@nospame That makes sense, appreciate the thoughts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/all-users-all-environments Change impacts all users on all environments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants