Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: add global notification component#996

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
kylecarbs merged 6 commits intomainfrombq/992/notifications
Apr 13, 2022
Merged

Conversation

BrunoQuaresma
Copy link
Collaborator

Closes#992

@BrunoQuaresmaBrunoQuaresma requested a review froma team as acode ownerApril 13, 2022 16:07
@BrunoQuaresmaBrunoQuaresma self-assigned thisApr 13, 2022
@codecov
Copy link

codecovbot commentedApr 13, 2022
edited
Loading

Codecov Report

Merging#996 (4a1a133) intomain (5ecc823) willincrease coverage by0.16%.
The diff coverage is56.97%.

@@            Coverage Diff             @@##             main     #996      +/-   ##==========================================+ Coverage   66.82%   66.99%   +0.16%==========================================  Files         241      250       +9       Lines       14549    14819     +270       Branches      115      131      +16     ==========================================+ Hits         9723     9928     +205- Misses       3854     3911      +57- Partials      972      980       +8
FlagCoverage Δ
unittest-go-macos-latest54.01% <ø> (+0.21%)⬆️
unittest-go-postgres-66.63% <ø> (+0.06%)⬆️
unittest-go-ubuntu-latest56.62% <ø> (+0.08%)⬆️
unittest-go-windows-202253.19% <ø> (?)
unittest-js61.24% <56.97%> (-0.33%)⬇️
Impacted FilesCoverage Δ
site/src/app.tsx0.00% <0.00%> (ø)
site/src/components/Snackbar/GlobalSnackbar.tsx27.27% <27.27%> (ø)
...ite/src/components/Snackbar/EnterpriseSnackbar.tsx50.00% <50.00%> (ø)
site/src/components/Snackbar/index.ts77.77% <77.77%> (ø)
site/src/components/Icons/ErrorIcon.tsx100.00% <100.00%> (ø)
site/src/hooks/events.ts100.00% <100.00%> (ø)
site/src/util/events.ts100.00% <100.00%> (ø)
coderd/workspaceagents.go59.82% <0.00%> (-3.58%)⬇️
peer/conn.go80.20% <0.00%> (-2.80%)⬇️
peerbroker/dial.go83.60% <0.00%> (ø)
... and16 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update5ecc823...4a1a133. Read thecomment docs.

@kylecarbs
Copy link
Member

I'm nervous about adding this, as I've seen our global notifications lead to errors that aren't attached to any user action or component. An example is the connection failures we display in v1.

Does anyone else feel similarly?

greyscaled reacted with thumbs up emoji

@BrunoQuaresma
Copy link
CollaboratorAuthor

BrunoQuaresma commentedApr 13, 2022
edited
Loading

@kylecarbs IMO this component itself is not the issue but how we were using it in V1. We will be cautious about the usage of this component for sure. It is also important to share more context:

  • We decided to move with this because of the deadline
  • Ideally, we want to have the feedback messages close to where the action is triggered but it would take some time to refactor the existing code from v1.
kylecarbs reacted with thumbs up emoji

@greyscaled
Copy link
Contributor

Something we could right now is only portdisplaySuccess. We had talked in slack about not wanting to portdisplayError as it was in v1, so very much agreed@kylecarbs

Copy link
Contributor

@greyscaledgreyscaled left a comment

Choose a reason for hiding this comment

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

A lot of this code can be cleaned up later --> I'll create a ticket to refactor it mentioning XService port.

Thanks for porting this@BrunoQuaresma

BrunoQuaresma reacted with heart emoji
@greyscaledgreyscaled mentioned this pull requestApr 13, 2022
2 tasks
@BrunoQuaresmaBrunoQuaresmaenabled auto-merge (squash)April 13, 2022 16:56
@BrunoQuaresmaBrunoQuaresmaenabled auto-merge (squash)April 13, 2022 18:19
@kylecarbskylecarbs merged commit300c6d0 intomainApr 13, 2022
@kylecarbskylecarbs deleted the bq/992/notifications branchApril 13, 2022 18:34
@missknissmisskniss added this to theV2 Beta milestoneMay 15, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@greyscaledgreyscaledgreyscaled approved these changes

Assignees

@BrunoQuaresmaBrunoQuaresma

Labels
None yet
Projects
None yet
Milestone
V2 Beta
Development

Successfully merging this pull request may close these issues.

Port GlobalNotifications component
4 participants
@BrunoQuaresma@kylecarbs@greyscaled@misskniss

[8]ページ先頭

©2009-2025 Movatter.jp