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 background color style #2326

Merged
merged 37 commits into from
Aug 11, 2023
Merged

fix background color style #2326

merged 37 commits into from
Aug 11, 2023

Conversation

JerryWu1234
Copy link
Contributor

@JerryWu1234 JerryWu1234 commented Jul 18, 2023

image
fix #2301

  • I've read and followed the contributing guide on how to create great pull requests.
  • I've updated the relevant documentation for any new or updated feature
  • I've linked relevant GitHub issue with "Closes #"
  • I've added a visual demonstration under the form of a screenshot or video

@JerryWu1234
Copy link
Contributor Author

if the inset was set, background was #f7f7f7, I think the preview mode follows the dev

@Janpot Janpot added the core Infrastructure work going on behind the scenes label Jul 18, 2023
@Janpot
Copy link
Member

Janpot commented Jul 18, 2023

I believe the preview is correct, it's the canvas that overlays a slight transparent grey when nothing is selected. The idea is to emphasize something is selected by slightly dimming the rest. It gives the illusion of a grey background.

We can start by disabling this overlay when nothing is selected. If this turns out to still be confusing we will disable it altogether and rely purely on the blue box to indicate selection.

@JerryWu1234 JerryWu1234 marked this pull request as draft July 18, 2023 10:04
@JerryWu1234 JerryWu1234 marked this pull request as ready for review July 18, 2023 10:59
@JerryWu1234
Copy link
Contributor Author

JerryWu1234 commented Jul 18, 2023

we will disable it altogether and rely purely on the blue box to indicate selection

I chose this way
image
image

@Janpot
Copy link
Member

Janpot commented Jul 19, 2023

If we don't want a background at all, you may as well remove this whole component

@JerryWu1234
Copy link
Contributor Author

JerryWu1234 commented Jul 19, 2023

If we don't want a background at all, you may as well remove this whole component

actually, I removed the whole component(PinholeOverlay.tsx) but the [chromium] › integration/rest-basic/index.spec.ts:36:5 › rest runtime basics this test will occur an error.
let me dive into deeper

@JerryWu1234
Copy link
Contributor Author

image

@Janpot
Copy link
Member

Janpot commented Jul 19, 2023

Yes, you may have to move the data-testid="page-overlay" one comonent up

@JerryWu1234
Copy link
Contributor Author

JerryWu1234 commented Jul 21, 2023

@Janpot
after my in-depth study,
We can’t maybe directly delete this part of PinholeOverlay, because it is not only for emphasizing something that is selected, but also for canceling the status of active when something is selected.

@JerryWu1234
Copy link
Contributor Author

JerryWu1234 commented Jul 21, 2023

I added a test for status switching

Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

If the PinholeOverlay does have utility and the only issue was the darkening of the UI I think just removing that darkening should do.

And thanks for adding a test for this! Adding some suggestions for the test:

test/integration/editor/index.spec.ts Outdated Show resolved Hide resolved
test/integration/editor/index.spec.ts Outdated Show resolved Hide resolved
test/integration/editor/index.spec.ts Outdated Show resolved Hide resolved
@JerryWu1234
Copy link
Contributor Author

JerryWu1234 commented Jul 31, 2023

@apedroferreira
please help approve this pr if it is mergeable.

Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

Almost there, just a few more important changes in the test!
Also just a nitpick, but feel free to use more vertical space in the test to separate bits of logic, it can really help improve the readability!

test/integration/editor/index.spec.ts Outdated Show resolved Hide resolved
test/integration/editor/index.spec.ts Outdated Show resolved Hide resolved
test/integration/editor/index.spec.ts Outdated Show resolved Hide resolved
@apedroferreira
Copy link
Member

apedroferreira commented Aug 1, 2023

With my comments above, here's how I would vertically space the test, order the variables and change variable names to improve readability and make the test as easy to understand as possible:

  const editorModel = new ToolpadEditor(page);

  await editorModel.goToPageById('K7SkzhT');

  await editorModel.waitForOverlay();

  const textField = editorModel.appCanvas.locator('input');
  const textFieldNodeHudTag = editorModel.appCanvas
    .getByTestId('node-hud-tag')
    .filter({ hasText: 'textField' });

  await clickCenter(page, textField);
  await expect(textFieldNodeHudTag).toBeVisible();

  const textFieldBoundingBox = await textField.boundingBox();
  await page.mouse.click(textFieldBoundingBox!.x + 150, textFieldBoundingBox!.y + 150);
  await expect(textFieldNodeHudTag).toBeHidden();

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 1, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 2, 2023
Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a lot!

@apedroferreira
Copy link
Member

apedroferreira commented Aug 11, 2023

@JerryWu1234 feel free to merge this!
Nevermind, I didn't know external contributors can't merge :D I'll merge it now, thanks again!

@apedroferreira apedroferreira merged commit f1edd0f into mui:master Aug 11, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Background color of canvas in edit and preview mode is different
3 participants