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

style: update overlay style on mobile #15760

Merged
merged 7 commits into from Mar 12, 2024
Merged

style: update overlay style on mobile #15760

merged 7 commits into from Mar 12, 2024

Conversation

PengBoUESTC
Copy link
Contributor

Description

make overlay style more flexible。
px is not friendly in mobile client。

before

after

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines, especially the Pull Request Guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Jan 31, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@PengBoUESTC PengBoUESTC requested a review from btea February 1, 2024 03:47
Comment on lines 142 to 145
.stack {
white-space: pre-wrap;
word-break: break-all;
}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this looks great. The stack trace paths can be long, and I think it's nicer if they're all within a single line and we can scroll it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if this looks great. The stack trace paths can be long, and I think it's nicer if they're all within a single line and we can scroll it.

normally, scroll-x in mobile modal is not good for developer or user , so I think that word-break is necessary

Copy link
Member

Choose a reason for hiding this comment

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

I'll let others chime in about this, but I think it's harder to read if the stack traces are line-wrapped.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer a scrollbar, too. I think it's useful to have the function names lined up vertically.

packages/vite/src/client/overlay.ts Outdated Show resolved Hide resolved
@bluwy bluwy changed the title chore: overlay style opt style: update overlay style on mobile Feb 6, 2024
@bluwy bluwy added the p2-nice-to-have Not breaking anything but nice to have (priority) label Feb 6, 2024
bluwy
bluwy previously approved these changes Feb 26, 2024
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Leaving my approval for the new width changes, but I'll still likely others thoughts on the stack trace line wrapping (#15760 (comment)). Here's the new screenshots for easier review:

Screeenshots image image

@PengBoUESTC
Copy link
Contributor Author

Leaving my approval for the new width changes, but I'll still likely others thoughts on the stack trace line wrapping (#15760 (comment)). Here's the new screenshots for easier review:

Screeenshots

ok , maybe we need more advices about that;

Copy link
Contributor

@chaejunlee chaejunlee left a comment

Choose a reason for hiding this comment

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

I totally agree with your change, however, I believe max-width and padding still should be static.
With large width monitor system, though it is pretty rare situation, using visual viewport metric lowers the experience.

I hope you can agree with me!

@@ -44,10 +44,11 @@ const template = /*html*/ `
.window {
font-family: var(--monospace);
line-height: 1.5;
width: 800px;
max-width: 80vw;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
max-width: 80vw;
width: 80vw;
max-width: 800px;

How about doing max-width: 800px; with width: 80vw;?
I believe that max-width should be a static value.
If we use max-width: 80vw;, it would expand infinitely and I believe it looks bad.

This applies to padding as well, however, I think we can use rem instead of px. What I found looked pretty good was padding: 1.5rem 2rem;

I am bringing up an extreme environment where screen width is 4000px wide, but I think you will get the point.

[visual viewport based]
Screenshot 2024-02-28 at 16 34 38

[max-width: 800px w/ rem padding]
Screenshot 2024-02-28 at 16 35 42

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I see here, I think it would be better to have the block margin of .window to be dynamic to viewport height, i.e. margin: 10vh auto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the problem is that, the overlay error info is not designed for Flexible screen, if U use big screen, the main problem maybe the font-size;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the problem is that, the overlay error info is not designed for Flexible screen, if U use big screen, the main problem maybe the font-size;

Copy link
Contributor

Choose a reason for hiding this comment

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

I get what you mean, but as we are introducing this change to support mobile viewport, we should consider the wider viewport as well.

If you think my opinion is unrelated to your PR, I will open another PR after this one being merged and continue the discussion! 😊

@patak-dev patak-dev added this to the 5.2 milestone Mar 5, 2024
@bluwy bluwy merged commit 4559ac0 into vitejs:main Mar 12, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants