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

fixed Toast is briefly displayed on page load #3725

Merged
merged 3 commits into from
Mar 27, 2024

Conversation

r3yc0n1c
Copy link
Contributor

fixes #2512

Summary:

  • Resolved the toast-display issue on page load in multiple pages like roadmap, myfeatures, newfeatures, and probably others.

Tests

  • Tested the toast display on roadmap, myfeatures, newfeatures, etc. pages
  • Tested correct workflow of the toast popups like "Link copied", "Some errors occurred", etc.

Screenshots

simplescreenrecorder-2024-03-20_20.48.38.mp4

This approach works on every page, but having a welcome toast on every page was not ideal. Previously, due to the initialization, the toast used to appear briefly like that. Thus, I've commented out the toast initialization with the welcome text in the spa and _base.

For now, I've considered initializing the toast only where needed, thus solving this issue.

I'm open to discussing and implementing any other approach. I would appreciate a review of this! Thanks!

cc: @DanielRyanSmith @jrobbins

Copy link

google-cla bot commented Mar 20, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link
Collaborator

@DanielRyanSmith DanielRyanSmith left a comment

Choose a reason for hiding this comment

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

This is very close, but some toast messages on various pages no longer display. Once the toast is added to the document in chromedash-app, it should be available to use for any system messages that are displayed.

templates/_base.html Outdated Show resolved Hide resolved
client-src/elements/chromedash-roadmap-page.js Outdated Show resolved Hide resolved
templates/spa.html Outdated Show resolved Hide resolved
@DanielRyanSmith
Copy link
Collaborator

This is looking like the solution we're looking for, but it seems some of the test reference files need to be regenerated, since they no longer have the chromedash-toast element in them. Take a look at the "Details" section of the python_tests check that has failed, and follow the instructions on the readme to update the test html renderings.

E.g. for one of the tests you'll need to uncomment this line and run npm test, which will regenerate the rendering, then comment it out again and make sure the test passes after. Then commit that change to the html rendering.

@r3yc0n1c
Copy link
Contributor Author

This is looking like the solution we're looking for, but it seems some of the test reference files need to be regenerated, since they no longer have the chromedash-toast element in them. Take a look at the "Details" section of the python_tests check that has failed, and follow the instructions on the readme to update the test html renderings.

E.g. for one of the tests you'll need to uncomment this line and run npm test, which will regenerate the rendering, then comment it out again and make sure the test passes after. Then commit that change to the html rendering.

fixed the html test pages and tested with npm test but this error pops up:

FAIL: test__normal (internals.approval_defs_test.FetchOwnersTest.test__normal)
We can fetch and parse an OWNERS file.  And reuse cached value.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/rey/.pyenv/versions/3.11.8/lib/python3.11/unittest/mock.py", line 1375, in patched
    return func(*newargs, **newkeywargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/rey/Documents/chromium-dashboard/internals/approval_defs_test.py", line 48, in test__normal
    mock_get.assert_called_once_with('https://example.com')
  File "/home/rey/.pyenv/versions/3.11.8/lib/python3.11/unittest/mock.py", line 950, in assert_called_once_with
    raise AssertionError(msg)
AssertionError: Expected 'get' to be called once. Called 2 times.
Calls: [call('https://example.com'), call('https://example.com')].

----------------------------------------------------------------------
Ran 752 tests in 81.140s

FAILED (failures=1, skipped=3)

> stop-emulator
> curl -X POST 'http://localhost:15606/shutdown'; pkill -f "CloudDatastore.jar"

Shutting down...
pkill: killing pid 99645 failed: Operation not permitted
Terminated

I've pushed the latest code for your ref.

@r3yc0n1c
Copy link
Contributor Author

Hi @DanielRyanSmith could you please review these changes?

Thanks!

Copy link
Collaborator

@DanielRyanSmith DanielRyanSmith left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@DanielRyanSmith DanielRyanSmith merged commit 3612489 into GoogleChrome:main Mar 27, 2024
7 checks passed
@r3yc0n1c
Copy link
Contributor Author

Thanks for the fix!

Thanks for merging 🙌! I want to continue contributing to Chromestatus for GSoC. Can you suggest more issues for me to work on?

@r3yc0n1c
Copy link
Contributor Author

Also, I think GitHub might not be the most suitable platform for small talk or quick discussions. Do you have another preferred method of communication where we can chat more easily? Alternatively, if email works better for you, we can continue our discussions via raja.majumdar.01@gmail.com.

Thanks!

@DanielRyanSmith
Copy link
Collaborator

Hello @r3yc0n1c - feel free to send me an email with questions you'd like to discuss. You should be able to find my contact email through the GSoC project definitions.

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.

Toast is briefly displayed on page load
2 participants