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.

Sign up for GitHub

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

Jump to bottom

fix(docs-infra): parent max-height IDE error panel visibility #54128

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(docs-infra): parent max-height IDE error panel visibility #54128

wants to merge 3 commits into from
+14 −2

Conversation

vladboisa
Copy link
Contributor

@vladboisa vladboisa commented Jan 28, 2024 •
edited

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Was:
image

Issue Number: #52760

What is the new behavior?

After adding overflow and max-height in percentage to parent container and, move to the bottom error box,
issue behavior was fixed
image

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@vladboisa vladboisa mentioned this pull request Jan 28, 2024
Open
@vladboisa vladboisa changed the title fix: parent max-height IDE error panel visibility fix(docs-infra): parent max-height IDE error panel visibility Jan 28, 2024
@vladboisa vladboisa force-pushed the fix-IDE-Panel-visibility branch 2 times, most recently from 3533f97 to 0569bff Compare January 28, 2024 16:58
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 28, 2024 •
edited

Deployed adev-preview for f2d9f4a to: https://ng-dev-previews-fw--pr-angular-angular-54128-adev-prev-c6p96pov.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

@JeanMeche
Copy link
Member

Hi, thanks for proposing a fix.

With your fix, the origin issue still stands, the error message hides the issues (line 17 and 19 aren't visible).

Screenshot 2024-01-28 at 18 47 16

@vladboisa
Copy link
Contributor Author

vladboisa commented Jan 28, 2024 •
edited

Hi, thanks for proposing a fix.

With your fix, the origin issue still stands, the error message hides the issues (line 17 and 19 aren't visible).

Maybe the best way to fix, is to move this error box to the right-side, by removing left: 0.5 rem and setting to max-width:50% ?
image

@jessicajaniuk jessicajaniuk added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release area: docs-infra Angular.dev application and infrastructure labels Jan 29, 2024
@ngbot ngbot bot added this to the Backlog milestone Jan 29, 2024
@twerske
Copy link
Contributor

twerske commented Jan 29, 2024

We wouldn't want to shift the width, the height should be probably a percent of the box - cc @bencodezen

@vladboisa
Copy link
Contributor Author

We wouldn't want to shift the width, the height should be probably a percent of the box - cc @bencodezen

So, my solution is fine? Or need to calc the percentage

@bencodezen
Copy link
Contributor

@vladboisa Your solution is a good start in terms of not letting the error messages take over too much of the editor.

That said, if the goal is to allow users to fix the errors while the popup is visible, this doesn't fix the issue.

The additional piece we need on this is to modify the height of the editor so that users can see the errors in question while also seeing the error box.

Screenshot 2024-01-29 at 9 39 04 AM

This is a quick prototype I did by modifying the height of the editor, but I know it'll need more dynamic calculations based on whether the error popup has appeared or not.

Let me know if you have any additional questions!

@bencodezen bencodezen self-requested a review January 29, 2024 17:40
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Jan 30, 2024
@vladboisa
Copy link
Contributor Author

vladboisa commented Feb 1, 2024 •
edited

@vladboisa Your solution is a good start in terms of not letting the error messages take over too much of the editor.

That said, if the goal is to allow users to fix the errors while the popup is visible, this doesn't fix the issue.

The additional piece we need on this is to modify the height of the editor so that users can see the errors in question while also seeing the error box.

This is a quick prototype I did by modifying the height of the editor, but I know it'll need more dynamic calculations based on whether the error popup has appeared or not.

Let me know if you have any additional questions!

@bencodezen I've added the animation, and make a modification to the height of the editor, when the error box is appears

@angular-robot angular-robot bot added area: docs Related to the documentation area: build & ci Related the build and CI infrastructure of the project labels Feb 25, 2024
@angular-robot angular-robot bot removed area: docs Related to the documentation area: build & ci Related the build and CI infrastructure of the project labels Mar 12, 2024
@vladboisa
fix(docs-infra): max-height IDE error panel visibility
b8dc0c3 
Remove max-height: 200px in ul child inline-errors-box, add the overflow & max-height in percentages to the parent for correct visualization

Fixes angular#52760

refactor(docs-infra): correct typo
Correct typo in comment

feat(docs-infra): modify the height of the editor
If error box are displayed, modify the height of the editor
@vladboisa
feat(docs-infra): add transition on editor wrapper
5eaf975 
Add the smooth animation when height of the container is changing
@vladboisa
fix(docs-infra): fix calculation of height editor
f2d9f4a 
Apply the min() function for set the smallest height

fix(docs-infra): move height into editor-wrapper

Move the calculation rule of height edit into editor-wrapper selector

fix(docs-infra): change has selector

Change the has selector

fix(docs-infra): change selector's for child

Changing the selector for test this solution

Fix
@vladboisa
Copy link
Contributor Author

@vladboisa Your solution is a good start in terms of not letting the error messages take over too much of the editor.

That said, if the goal is to allow users to fix the errors while the popup is visible, this doesn't fix the issue.

The additional piece we need on this is to modify the height of the editor so that users can see the errors in question while also seeing the error box.
Let me know if you have any additional questions!

@bencodezen @twerske
I think now my solution is work's fine.
Thanks for your time and the opportunity to contribute in Angular community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twerske twerske Awaiting requested review from twerske

@bencodezen bencodezen Awaiting requested review from bencodezen

@alxhub alxhub Awaiting requested review from alxhub

@AndrewKushnir AndrewKushnir Awaiting requested review from AndrewKushnir

@atscott atscott Awaiting requested review from atscott

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer adev: preview area: docs-infra Angular.dev application and infrastructure detected: feature PR contains a feature commit target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@vladboisa @JeanMeche @twerske @bencodezen @jessicajaniuk