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 bugs #542

Merged
merged 8 commits into from
Jul 31, 2023
Merged

fix: ui bugs #542

merged 8 commits into from
Jul 31, 2023

Conversation

KristinAoki
Copy link
Member

@KristinAoki KristinAoki commented Jul 27, 2023

Enable contentstore.new_studio_mfe.use_new_advanced_settings_page to view page.

Addresses the following issues:

  1. Aside wider than screen
  2. success banner should only last for 15 seconds
  3. tooltip not dark variant
  4. info icon not outline
  5. alpha order of advanced settings
  6. Info icon not on same line as card header

@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Patch coverage: 95.45% and project coverage change: +1.27% 🎉

Comparison is base (8bfc3f2) 78.20% compared to head (d9ee6cd) 79.48%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
+ Coverage   78.20%   79.48%   +1.27%     
==========================================
  Files         145      145              
  Lines        2698     2710      +12     
  Branches      632      636       +4     
==========================================
+ Hits         2110     2154      +44     
+ Misses        558      526      -32     
  Partials       30       30              
Files Changed Coverage Δ
src/custom-pages/CustomPages.jsx 81.57% <ø> (ø)
src/advanced-settings/setting-card/SettingCard.jsx 91.30% <87.50%> (-8.70%) ⬇️
src/advanced-settings/AdvancedSettings.jsx 89.15% <100.00%> (+6.17%) ⬆️
src/advanced-settings/data/thunks.js 84.09% <100.00%> (+44.09%) ⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jesperhodge jesperhodge left a comment

Choose a reason for hiding this comment

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

The changes looks good, but I found a little problem I think, could you verify whether this appears to you too?

There's a noticable difference in performance. There's already some tiny lag on the master branch for advanced settings, but it get's quite sluggish when you checkout this branch. I switched back and forth between branches a couple of times and I can tell that it's related to something on this branch. At least that's that way on my browser.

I assume we will have to look into the performance graphs of the browser console, do some throttling, and see what's actually happening. I can do some of this as well, though I would like confirmation that it's not just happening on my own machine.

Basically I noticed it when I typed anything in the fields quickly, there was a noticable lag before the input appeared, and it was sluggish and bumpy (type 3 characters quickly and then there's a lag and then all 3 characters appear at once, for example).

I assume this has either something to do with the new transitions, rendering cycle, or maybe a memory leak.

Copy link
Contributor

@rayzhou-bit rayzhou-bit left a comment

Choose a reason for hiding this comment

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

lgtm!

@jesperhodge
Copy link
Member

The changes looks good, but I found a little problem I think, could you verify whether this appears to you too?

There's a noticable difference in performance. There's already some tiny lag on the master branch for advanced settings, but it get's quite sluggish when you checkout this branch. I switched back and forth between branches a couple of times and I can tell that it's related to something on this branch. At least that's that way on my browser.

I assume we will have to look into the performance graphs of the browser console, do some throttling, and see what's actually happening. I can do some of this as well, though I would like confirmation that it's not just happening on my own machine.

Basically I noticed it when I typed anything in the fields quickly, there was a noticable lag before the input appeared, and it was sluggish and bumpy (type 3 characters quickly and then there's a lag and then all 3 characters appear at once, for example).

I assume this has either something to do with the new transitions, rendering cycle, or maybe a memory leak.

Here are screenshots of performance analysis for this branch, note the red "keydown" and "keyup" events:
Screenshot 2023-07-27 at 2 42 38 PM

And for master, to compare:
Screenshot 2023-07-27 at 2 44 24 PM

@KristinAoki KristinAoki dismissed jesperhodge’s stale review July 31, 2023 21:19

Addressed changes and need to unblock merge

@KristinAoki KristinAoki merged commit 9f4422d into master Jul 31, 2023
5 checks passed
@KristinAoki KristinAoki deleted the KristinAoki/fix-ui-bugs branch July 31, 2023 21:23
PKulkoRaccoonGang pushed a commit to raccoongang/frontend-app-course-authoring that referenced this pull request Aug 1, 2023
wowkalucky pushed a commit to raccoongang/frontend-app-course-authoring that referenced this pull request Aug 1, 2023
* chore: add paragon messages (openedx#530)

* chore: add paragon messages (openedx#530) (openedx#534)

Co-authored-by: Mashal Malik <107556986+Mashal-m@users.noreply.github.com>

* feat: make placeholder depend on api response

* feat: include paragon in atlas pull (openedx#538)

This pull request is part of the [FC-0012 project](https://openedx.atlassian.net/l/cp/XGS0iCcQ) which is sparked by the [Translation Infrastructure update OEP-58](https://open-edx-proposals.readthedocs.io/en/latest/architectural-decisions/oep-0058-arch-translations-management.html#specification).

* fix: ui bugs (openedx#542)

---------

Co-authored-by: Mashal Malik <107556986+Mashal-m@users.noreply.github.com>
Co-authored-by: sundasnoreen12 <72802712+sundasnoreen12@users.noreply.github.com>
Co-authored-by: Omar Al-Ithawi <i@omardo.com>
Co-authored-by: Kristin Aoki <42981026+KristinAoki@users.noreply.github.com>
vladislavkeblysh pushed a commit to raccoongang/frontend-app-course-authoring that referenced this pull request Aug 2, 2023
* chore: add paragon messages (openedx#530)

* chore: add paragon messages (openedx#530) (openedx#534)

Co-authored-by: Mashal Malik <107556986+Mashal-m@users.noreply.github.com>

* feat: make placeholder depend on api response

* feat: include paragon in atlas pull (openedx#538)

This pull request is part of the [FC-0012 project](https://openedx.atlassian.net/l/cp/XGS0iCcQ) which is sparked by the [Translation Infrastructure update OEP-58](https://open-edx-proposals.readthedocs.io/en/latest/architectural-decisions/oep-0058-arch-translations-management.html#specification).

* fix: ui bugs (openedx#542)

---------

Co-authored-by: Mashal Malik <107556986+Mashal-m@users.noreply.github.com>
Co-authored-by: sundasnoreen12 <72802712+sundasnoreen12@users.noreply.github.com>
Co-authored-by: Omar Al-Ithawi <i@omardo.com>
Co-authored-by: Kristin Aoki <42981026+KristinAoki@users.noreply.github.com>
snglth pushed a commit to Abstract-Tech/community-theme-course-authoring that referenced this pull request Jan 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants