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

misc: Update styling of Runs Page #26406

Merged
merged 13 commits into from
Apr 6, 2023
Merged

Conversation

mike-plummer
Copy link
Contributor

Additional details

  • Updating status icons to match other areas of the app (specs list, debug page)
    • Use shared component to reduce complexity and use icon assets from design system
  • Wrap run status as viewport shrinks to improve responsive layout
  • Add link params

Steps to test

Review RunCard.cy.tsx component test. Changes can be viewed in the following tests:

  • "renders expected icon for each run status"
  • "renders with all run information on small viewport"

How has the user experience changed?

Screenshot 2023-04-03 at 10 09 41 AM

Screenshot 2023-04-03 at 10 29 56 AM

PR Tasks

* Use new status icons to match other areas of the app
* Improve responsiveness of UI on smaller viewports
* Update link params
* Use new status icons to match other areas of the app
* Improve responsiveness of UI on smaller viewports
* Update link params
@mike-plummer mike-plummer changed the title fix: Update styling of Runs Page misc: Update styling of Runs Page Apr 3, 2023
@cypress
Copy link

cypress bot commented Apr 3, 2023

30 flaky tests on run #45298 ↗︎

0 26909 1281 0 Flakiness 30

Details:

Fix test
Project: cypress Commit: ccf3d94997
Status: Passed Duration: 19:42 💡
Started: Apr 5, 2023 8:22 PM Ended: Apr 5, 2023 8:42 PM
Flakiness  commands/net_stubbing.cy.ts • 1 flaky test • 5x-driver-firefox

View Output Video

Test Artifacts
network stubbing > intercepting request > can delay and throttle a StaticResponse Output
Flakiness  e2e/origin/commands/assertions.cy.ts • 1 flaky test • 5x-driver-firefox

View Output Video

Test Artifacts
cy.origin assertions > #consoleProps > .should() and .and() Output
Flakiness  cypress/cypress.cy.js • 3 flaky tests • 5x-driver-firefox

View Output Video

Test Artifacts
... > correctly returns currentRetry Output
... > correctly returns currentRetry Output
... > correctly returns currentRetry Output
Flakiness  e2e/origin/navigation.cy.ts • 1 flaky test • 5x-driver-electron

View Output Video

Test Artifacts
delayed navigation > errors > redirects to an unexpected cross-origin and calls another command in the cy.origin command Output Video
Flakiness  e2e/origin/user_agent_override.cy.ts • 1 flaky test • 5x-driver-electron

View Output Video

Test Artifacts
user agent override > persists modified user agent after cy.go Output Video

The first 5 flaky specs are shown, see all 18 specs in Cypress Cloud.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@mike-plummer
Copy link
Contributor Author

Hmm, Percy seems to be doing some odd things with wrapping the Run Results section over and not spacing properly that I can't reproduce locally. Works fine for me locally in Chrome, Edge, FF, and Safari. Any ideas why this could be happening @marktnoonan ?

Copy link
Contributor

@warrensplayer warrensplayer left a comment

Choose a reason for hiding this comment

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

All looks good except if there is a long commit message:
Screenshot 2023-04-04 at 11 09 02 AM

I played around with the code, but the only solution I can see is to not use the ListRowHeader component so that you can better control how the commit message is rendered.

@warrensplayer
Copy link
Contributor

Hmm, Percy seems to be doing some odd things with wrapping the Run Results section over and not spacing properly that I can't reproduce locally. Works fine for me locally in Chrome, Edge, FF, and Safari.

@mike-plummer I am not able to reproduce locally either. One thing to try might be to replace the span you added (<span class="flex flex-wrap gap-30px justify-between">) with div. Percy seems to be wrapping early and then not honoring your gap of 30px that is there to offset the negative top margin.

My hope also is that rewriting the component to not use ListRowHeader will fix it as well.

@mike-plummer mike-plummer requested a review from a team April 5, 2023 13:55
Copy link
Contributor

@warrensplayer warrensplayer left a comment

Choose a reason for hiding this comment

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

Everything looking good now.

Extra credit would be some documentation or a test for the addition to the ListRowHeader component.

@warrensplayer
Copy link
Contributor

@mike-plummer I like the test update. Can I approve the PR twice? ✅ ✅

@lmiller1990 lmiller1990 self-requested a review April 6, 2023 03:21
Copy link
Contributor

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

Beautiful, looks great!

@lmiller1990 lmiller1990 removed the request for review from marktnoonan April 6, 2023 03:22
@lmiller1990
Copy link
Contributor

Percy looks fine to me - the update (right hand) seems like it's correct now.

image

@lmiller1990
Copy link
Contributor

lmiller1990 commented Apr 6, 2023

Can definitely look into Percy if we think there's still issues - might go ahead and merge this up since the code and UI looks correct to me. Maybe the <span> as suggested by @warrensplayer solved the issue?

Either way no blocker, let's ship it 👍

@lmiller1990 lmiller1990 merged commit 89c23e0 into develop Apr 6, 2023
@lmiller1990 lmiller1990 deleted the mikep/26180-runs-page-ui-updates branch April 6, 2023 03:27
astone123 pushed a commit to kgroat/cypress that referenced this pull request Apr 19, 2023
* fix: Update styling of Runs Page

* Use new status icons to match other areas of the app
* Improve responsiveness of UI on smaller viewports
* Update link params

* misc: Update styling of Runs Page

* Use new status icons to match other areas of the app
* Improve responsiveness of UI on smaller viewports
* Update link params

* Add changelog

* Fix e2e test

* Center run results when not wrapped

* Support wrapping with long commit message

* Fix changelog

* Add test case for new slot

* Fix test
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.

ACI - Runs page UI Updates
3 participants