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

feat(keyAutoAdd): show notification with connect progress #634

Merged
merged 6 commits into from
Oct 2, 2024
Merged

feat(keyAutoAdd): show notification with connect progress #634

merged 6 commits into from
Oct 2, 2024

Conversation

sidvishnoi
Copy link
Member

@sidvishnoi sidvishnoi commented Sep 30, 2024 •
edited
Loading

Context

Changes proposed in this pull request

  • Add a notification UI as a new extension page.
    • The page is injected as iframe as a part of key-auto add process. It's hidden by default.
    • On waiting for login (or any step that needs user interaction), we show the small notification UI (AppNotification) in top-right on page.
    • Otherwise, we show a full screen layover (AppFullScreen) to avoid intimidating user from automated actions we're taking.
    • Each step can decide what "notification size" it needs to use, or continue using the one set by previous step.
  • We keep posting the progress to the background process port from content script. For progress events, we re-emit that to the notification page's port.
  • We inform notification page App of notification size via iframe's URL hashchange events (useUIMode).

Demo: #634 (comment)

In future:

  • Better organize shared components in popup and upcoming pages.
  • Consider replacing React (95KB) with Preact (< 15KB) as each new page will add to bundle size.

@github-actions github-actions bot added area: content Improvements or additions to extension content script area: background Improvements or additions to extension background script area: popup Improvements or additions to extension popup labels Sep 30, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 30, 2024 •
edited
Loading

Extension builds preview

Name Link
Latest commit bf85589
Latest job logs Run #11144274362
BadgeDownload
BadgeDownload

@sidvishnoi
Copy link
Member Author

sidvishnoi commented Sep 30, 2024 •
edited
Loading

Added around 110KB to zip 😬 React!!?

Update: reduced ~50KB with fc095da

@sidvishnoi
Copy link
Member Author

sidvishnoi commented Oct 1, 2024 •
edited
Loading

Demo (with some artificial delay added to each step):

Screencast.from.01-10-24.06.03.14.PM.IST.webm

@sidvishnoi sidvishnoi marked this pull request as ready for review October 1, 2024 14:38
Base automatically changed from setup-page/key-share to main October 2, 2024 08:26
@sidvishnoi sidvishnoi removed the request for review from lengyel-arpad85 October 2, 2024 08:27
@sidvishnoi sidvishnoi changed the base branch from main to setup-page/key-share October 2, 2024 08:29
@sidvishnoi sidvishnoi changed the base branch from setup-page/key-share to main October 2, 2024 08:38
@github-actions github-actions bot added the area: documentation Improvements or additions to documentation label Oct 2, 2024
@sidvishnoi sidvishnoi removed the area: documentation Improvements or additions to documentation label Oct 2, 2024
raducristianpopa
raducristianpopa reviewed Oct 2, 2024
View reviewed changes
src/content/keyAutoAdd/lib/keyAutoAdd.ts Outdated Show resolved Hide resolved
src/content/keyAutoAdd/lib/keyAutoAdd.ts Show resolved Hide resolved
src/pages/progress-connect/components/Countdown.tsx Outdated Show resolved Hide resolved
src/content/keyAutoAdd/lib/keyAutoAdd.ts Show resolved Hide resolved
src/content/keyAutoAdd/lib/keyAutoAdd.ts Outdated
@@ -87,6 +174,7 @@ export class KeyAutoAdd {
}
const details = this.errorToDetails(error);
this.setStatus(stepIdx, 'error', { details: details });
await sleep(2000);
Copy link
Member

Choose a reason for hiding this comment

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

We probably need to remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, I think I want user to see the step failing. Or maybe the redirect to welcome screen showing right error message be more than enough?

Copy link
Member

Choose a reason for hiding this comment

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

If we add another value for the success parameter in the welcome screen I don't think we should need it at the moment. It depends on the UI - if we want the "loading bar". If we go for list, waiting for a couple of seconds would probably be okay.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, let's remove this for now. Can add again if needed.

src/manifest.json Show resolved Hide resolved
@github-actions github-actions bot added the area: documentation Improvements or additions to documentation label Oct 2, 2024
@sidvishnoi sidvishnoi merged commit b38ad6d into main Oct 2, 2024
9 checks passed
@sidvishnoi sidvishnoi deleted the connecting-notification branch October 2, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raducristianpopa raducristianpopa raducristianpopa approved these changes

Assignees
No one assigned
Labels
area: background Improvements or additions to extension background script area: content Improvements or additions to extension content script area: documentation Improvements or additions to documentation area: popup Improvements or additions to extension popup
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Setup]: Show auto-key add notification to user
2 participants
@sidvishnoi @raducristianpopa