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

fix: add padding to bottom of global mode list #22112

Merged
merged 15 commits into from
Jun 30, 2022

Conversation

viniciuspietscher
Copy link
Contributor

@viniciuspietscher viniciuspietscher commented Jun 5, 2022

This PR was Co-authored-by: Mark Noonan [email protected]

User facing changelog

Add 24px of padding to bottom of global mode list.

Additional details

  • Why was this change necessary?
    Projects list on GlobalPage were flush with the end of the page.

  • What is affected by this change?
    GlobalPage UI

Steps to test

Add a small viewport with cy.viewport() on packages/launchpad/src/global/GlobalPage.cy.tsx

How has the user experience changed?

Before:
before

After:
after

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jun 5, 2022

Thanks for taking the time to open a PR!

@CLAassistant
Copy link

CLAassistant commented Jun 5, 2022

CLA assistant check
All committers have signed the CLA.

@viniciuspietscher viniciuspietscher marked this pull request as ready for review June 5, 2022 22:41
@lmiller1990 lmiller1990 self-requested a review June 6, 2022 09:55
Copy link
Contributor

@tbiethman tbiethman left a comment

Choose a reason for hiding this comment

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

Fix looks good and addresses the immediate issue 👍

One thought I had is that we could add this padding globally within the launchpad, like we do for the horizontal padding:

class="px-24px pt-86px"

This would ensure that all components rendered within Main (including the project list) would have some level of bottom padding. But it would also add more padding to components that already have their own, so we'd have to double check for those.

@marktnoonan thoughts on this approach? If we want to circle back around with another issue to make the padding consistent, I'd be fine with that too. The project list is the most egregious at this point, but the browser selection and error views are also flush with the window when scrolled:

Screen Shot 2022-06-06 at 12 34 56 PM

Screen Shot 2022-06-06 at 12 34 20 PM

@marktnoonan
Copy link
Contributor

That's a good suggestion @tbiethman. @viniciuspietscher why don't you go ahead and make that change in Main.vue. Let's drop the component test changes from this PR and let Percy detect if the padding at the bottom of global mode changes.

@viniciuspietscher
Copy link
Contributor Author

That's a good suggestion @tbiethman. @viniciuspietscher why don't you go ahead and make that change in Main.vue. Let's drop the component test changes from this PR and let Percy detect if the padding at the bottom of global mode changes.

@marktnoonan updated the PR with the suggested changes. 👍

Copy link
Contributor

@ryanjwilke ryanjwilke left a comment

Choose a reason for hiding this comment

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

This does look like an improvement from what we have today.

@marktnoonan I would say that we should have a look together at the padding and scrollbars together as I've seen a lot of things that don't quite match up to our expectations and it's not easy to show some of these things in Figma either.

@lmiller1990
Copy link
Contributor

lmiller1990 commented Jun 27, 2022

PRs from external contributors consistently exhibit failures on these 7 tests link to CI. I don't know why, but it has happened multiple times recently, and I'd like to find out why, it's blocking lots of great contributions. Posting this here to have something to refer to while debugging this issue.

Edit: might be fixed: #22326

  • Another example. PR. CI.
  • Another example 2: PR. CI.
  • Another example 3: PR. CI

Even if I push an empty commit, it's the same. The problem seems to be isolated to CI runs with containers initially created by an external contributor.

image

@cypress
Copy link

cypress bot commented Jun 29, 2022



Test summary

37681 0 456 0Flakiness 11


Run details

Project cypress
Status Passed
Commit a801cd2
Started Jun 29, 2022 3:45 PM
Ended Jun 29, 2022 4:01 PM
Duration 16:34 💡
OS Linux Debian - 10.11
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

actions/click.cy.js Flakiness
1 ... > scroll-behavior > can scroll to and click elements in html with scroll-behavior: smooth
xhr.cy.js Flakiness
1 ... > logs Method, Status, URL, and XHR
2 ... > logs response
3 ... > logs request + response headers
4 ... > logs response
This comment includes only the first 5 flaky tests. See all 11 flaky tests in the Cypress Dashboard.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

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.

Add padding to bottom of global mode screen so UI is not flush against bottom
7 participants