-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[Mobile] Center Confirmation Dialog Texts #7492
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7492 +/- ##
==========================================
- Coverage 55.53% 55.15% -0.39%
==========================================
Files 672 672
Lines 26962 26962
Branches 2620 2620
==========================================
- Hits 14973 14870 -103
- Misses 11271 11374 +103
Partials 718 718
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Is this visual change covered by any visual tests? |
7bc4d9f
to
11990f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -3,7 +3,6 @@ | |||
|
|||
import { devices } from '@playwright/test'; | |||
const MAX_FAILURES = 5; | |||
const NUM_WORKERS = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
driveby: reduce test flakiness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice. yeah 1 should be enough here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one small non blocking suggestion
@@ -57,3 +53,23 @@ test('Verify that user can search @mobile', async ({ page }) => { | |||
await page.getByTitle('Collapse Browse Pane').click(); | |||
await expect(page.getByRole('main').getByText('Parent Display Layout')).toBeVisible(); | |||
}); | |||
test('Remove Object and confirmation dialog @mobile', async ({ page }) => { | |||
await page.goto('./'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can pull these page.goto('./');
out and into a test.beforeEach()
- Fixed an oversight that caused the top of form dialogs to be scrolled out of view by default. - Fixed approach to vertical centering for `-fit` type confirmation dialogs. - Reduced size of confirmation dialog icons. - Smoke tested in Chrome mobile emulator in a large variety of mobile viewport sizes and orientations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After finding some newly introduced issues, have pushed up new changes to address. With that, LGTM.
Problem related to extra margin being introduced to view large/preview/properties dialogs should be fixed. Visual tests should now pass... |
Closes #7347
Describe your changes:
All Submissions:
Author Checklist
type:
label? Note: this is not necessarily the same as the original issue.Reviewer Checklist