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: add button to get extension #417

Merged
merged 1 commit into from
Aug 11, 2020
Merged

feat: add button to get extension #417

merged 1 commit into from
Aug 11, 2020

Conversation

hstove
Copy link
Contributor

@hstove hstove commented Jun 9, 2020

Adds a button to the bottom of the 'choose account' screen to get the extension.

Figma designs

Right now, when you click the button, nothing happens. We don't have the extension published, yet.

  • The button should show the right icon & name if you're on Chrome or Firefox (or Chrome clones like Brave)
  • It should just show the 'Powered by Blockstack' icon if you're on a different browser (or iOS Chrome).

@hstove hstove requested a review from kyranjamie June 9, 2020 15:30
@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2020

Hey! If you'd like to test out the changes in this PR, check out our demo app. https://deploy-preview-417--authenticator-demo.netlify.com/

@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2020 •
edited
Loading

Size Change: 0 B

Total Size: 418 kB

ℹ️ View Unchanged
Filename Size Change
packages/connect/dist/bundle.js 356 kB 0 B
packages/connect/dist/connect.cjs.development.js 13.7 kB 0 B
packages/connect/dist/connect.cjs.production.min.js 10.4 kB 0 B
packages/connect/dist/connect.esm.js 13.5 kB 0 B
packages/connect/dist/connect.umd.development.js 13.9 kB 0 B
packages/connect/dist/connect.umd.production.min.js 10.5 kB 0 B
packages/connect/dist/index.js 143 B 0 B

compressed-size-action

kyranjamie
kyranjamie reviewed Jun 9, 2020
View reviewed changes
packages/app/src/components/extension-button.tsx Show resolved Hide resolved
packages/ui/src/icons/chrome-icon.tsx Outdated
@@ -0,0 +1,228 @@
import * as React from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Debating whether this belongs in ui. Is it domain-agnostic? 🤔 Maybe, I guess. But could also be seen as specific to this feature, thus belonging in app.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I see the argument for that! I've kind of liked simply throwing all new icons into UI, it just lets us work in one place. Would be curious to hear @aulneau 's opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am generally in favor of all icons being in the ui lib, because they truly are generic, typically at least. It helps force consistency for iconography. I don't think we lose anything by adding it.

packages/ui/src/icons/firefox-icon.tsx Outdated
@@ -0,0 +1,233 @@
import * as React from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is your IDE auto completing React as a * import? It's a default export anyway right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No - whenever I make a new icon, I copy/paste the first few lines from an existing icon file, which must have also had this. I'll update!

Copy link
Contributor

Choose a reason for hiding this comment

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

facebook/react#18102

@markmhendrickson
Copy link
Collaborator

@hstove Should we move this back to development if it's in draft still? Or is it ready to assign reviewers?

@markmhendrickson markmhendrickson added this to the UserX 2020 Sprint 2 milestone Jun 18, 2020
aulneau
aulneau reviewed Jun 18, 2020
View reviewed changes
Copy link
Contributor

@aulneau aulneau left a comment

Choose a reason for hiding this comment

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

I think for icons generally I prefer to use solid colored ones, and treat them as color="currentColor" so that they will take on whatever color we want them to. I'd be concerned about these icons working well in a light/dark mode environment.

@hstove
Copy link
Contributor Author

hstove commented Jun 18, 2020

Ok, that makes sense. So is that an argument for keeping this in app since it doesn't follow the pattern of our other icons? Or that we should consider redesigning to only use solid color icons?

@hstove
Copy link
Contributor Author

hstove commented Jun 18, 2020

Update: I refactored all the import * as React calls (mostly in UI), and moved the icons into the app/components/icons folder, which I think does make sense.

@hstove hstove force-pushed the feat/extension-button branch 2 times, most recently from ebd09fe to e54bdd7 Compare June 18, 2020 15:38
@hstove
Copy link
Contributor Author

hstove commented Jun 18, 2020

Ok, I reverted the import * from React stuff, but still moved the icons.

@hstove hstove force-pushed the feat/extension-button branch 2 times, most recently from 6082603 to c1409fa Compare June 18, 2020 16:00
@hstove hstove marked this pull request as ready for review June 30, 2020 13:29
@hstove hstove reopened this Jul 29, 2020
@hstove
Copy link
Contributor Author

hstove commented Jul 29, 2020

@timstackblock it looks like you accidentally closed this PR, instead of the issue! I'm re-opening it, and I've added the extension button to the 'sign in' popup, which relates to #471

@github-actions
Copy link
Contributor

github-actions bot commented Jul 29, 2020 •
edited
Loading

Deploy previews

Deploy previews for blockstack-app and blockstack-test-app are ready!

Built with commit 92855f2.

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3c611ea:

Sandbox Source
angry-allen-jdit7 Configuration

@hstove hstove merged commit f0ba354 into master Aug 11, 2020
@kyranjamie kyranjamie deleted the feat/extension-button branch June 14, 2021 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aulneau aulneau aulneau left review comments

@kyranjamie kyranjamie kyranjamie approved these changes

Assignees

@timstackblock timstackblock

Labels
None yet
Projects
None yet
Milestone
2020 W27-W29
Development

Successfully merging this pull request may close these issues.
5 participants
@hstove @markmhendrickson @kyranjamie @aulneau @timstackblock