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(UI): minimize the fixed contents upper the editor #54978

Conversation

mmatsumoto1026
Copy link
Contributor

@mmatsumoto1026 mmatsumoto1026 commented May 24, 2024

Checklist:

Partially address issues #53660 and #49054

This PR was opened to minimize the fixed contents upper the editor such as breadcrumb and a file display button in monaco-editor-tabs in Step challenges so that partially addressing the original issues as first step.

When the viewport height is equal to or less than 600px 900px mobile layout is displayed, the breadcrumb upper the editor moves into the description-container in the Step challenges.

On the other hand, if there is only one file to display on mobile layout, monaco-editor-tabs will be empty and the button to switch the file display will not be displayed .

By using gitpod, the code was tested with following environment.

  1. Windows 11 Home PC using Chrome, Firefox, Edge, and Brave browser, or, webkit within the Playwright.
  2. Android Studio's emulator(Nexus One with Android API 33 system image and Pixel 6 Pro with Android API 34 system image) using Chrome.
  3. iPhone SE (iOS 15.7.8) using Chrome and Firefox browser.

@mmatsumoto1026 mmatsumoto1026 requested a review from a team as a code owner May 24, 2024 18:52
@github-actions github-actions bot added the platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. label May 24, 2024
@moT01 moT01 added the status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. label May 24, 2024
client/src/templates/Challenges/classic/show.tsx Outdated Show resolved Hide resolved
client/src/templates/Challenges/classic/editor.tsx Outdated Show resolved Hide resolved
client/src/components/layouts/default.tsx Outdated Show resolved Hide resolved
@Sembauke Sembauke added status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP and removed status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. labels May 28, 2024
@Sembauke Sembauke added status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending. and removed status: waiting update To be applied to PR if a maintainer/reviewer has left a feedback and follow up is needed from OP labels Jun 3, 2024
client/config/misc.ts Outdated Show resolved Hide resolved
@Sembauke
Copy link
Member

Sembauke commented Jun 6, 2024

@ahmaxed Can you take a look at this?

@ahmaxed ahmaxed self-assigned this Jun 7, 2024
@bbsmooth
Copy link
Contributor

bbsmooth commented Jun 8, 2024

This is definitely an improvement. At 400% zoom (e.g. 320x256 px viewport) I can see about 4.5 lines of code instead of three, so we gained almost two lines. I'm wondering if we can do better. We could remove some of the top/bottom padding around both the tablist and file buttons. And we could remove some of the top/bottom padding in the tabs and buttons themselves. Yes, things will be a little more scrunched up in the header, but at such extremes zoom levels I think that editor space is more important than aesthetics. We could make these more extreme changes a smaller height, such as beginning at 600 or even 500px.

400-zoom

@mmatsumoto1026
Copy link
Contributor Author

Thanks for your review and comment, @bbsmooth.

I'm wondering if we can do better. We could remove some of the top/bottom padding around both the tablist and file buttons. And we could remove some of the top/bottom padding in the tabs and buttons themselves.

I definitely agree with you.

Although, as you mentioned, with removing the paddings, the fixed contents above the editor will be scrunched up, and I personally think that this will increase the possibility that users will tap the unintended button or tab on ordinary small devices, which can reduce usability.

Therefore, the EX_SMALL_VIEWPORT_HEIGHT = 300; and @media screen and (max-height: 300px) are defined and used to trigger making the spaces less.

The 300 thresh was used:

  1. to ensure the space removing will occur when the camper zoom in to 400% on laptop PC(but only considering for #mobile-layout).
  2. to avoid unexpected space removing on smartphones (such as iPhone 13 PRO and Google Pixel 6 pro) when the software keyboard is opened. Actually, on extremely small devices such as iPhone SE or Nexus One, the space removing will occur when the software keyboard was opened, but without the behavior, these devices almost can not display the code editor part.

I think the thresh value should receive more thorough review, as I have only used Android Studio's emulator and Chrome's mobile simulator to determine this value.

With removing some of the paddings, the camper can see six lines of code when opening the multifile editor and zoomed in 400% as shown in below image.

After removing some paddings around both the tablist and file buttons

@ahmaxed
Copy link
Member

ahmaxed commented Jun 10, 2024

Thanks for optimizing the mobile UI.
I might be missing something, but this is what I get for the first challenge on mobile and desktop. The breadcrumbs are not in the instructions on mobile and the index.html button is missing on desktop. file buttons should be displayed on desktop in case users wants to open or close a pane completely.

Screenshot 2024-06-10 at 4 08 48 PM Screenshot 2024-06-10 at 4 17 29 PM

@mmatsumoto1026
Copy link
Contributor Author

mmatsumoto1026 commented Jun 11, 2024

Thanks for your review, @ahmaxed.

The two issues you mentioned are occured with my misunderstanding of latest original issue report of yours, especially
I oversighted that viewport height of the mobile layout snapshot of the original issue (It was tall enough to contain all mobile devices).
Therefore, the fixed code was pushed to address all issues mentioned in this PR.

@ahmaxed
Copy link
Member

ahmaxed commented Jun 12, 2024

This feature would go greatly with a unit test so we could lock it in.

@github-actions github-actions bot added the scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. label Jun 13, 2024
@mmatsumoto1026
Copy link
Contributor Author

mmatsumoto1026 commented Jun 13, 2024

I'm not sure whether this is the best approach, with using the isMobile property, the e2e test for breadcrumb was updated for this new feature.

@ahmaxed
Copy link
Member

ahmaxed commented Jun 18, 2024

I'm not sure whether this is the best approach, with using the isMobile property, the e2e test for breadcrumb was updated for this new feature.

I will leave this one to our playwright expert: @Sembauke.

Bootstrap has just been removed so I recommend merging main and resolving the conflicts.

Other than that this should be good to go.

@Sembauke Sembauke merged commit 2847c7c into freeCodeCamp:main Jun 21, 2024
17 checks passed
@mmatsumoto1026 mmatsumoto1026 deleted the fix/minimize-the-fixed-contents-upper-the-editor branch June 21, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: learn UI side of the client application that needs familiarity with React, Gatsby etc. scope: tools/scripts Scripts for supporting dev work, generating config and build artifacts, etc. status: waiting review To be applied to PR's that are ready for QA, especially when additional review is pending.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants