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(wallet/ui): style connection component better #5984

Merged
merged 5 commits into from
Aug 18, 2022

Conversation

samsiegart
Copy link
Contributor

const pathStyle = { fill: '#bb2d40' };

return (
<svg
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason loading a static resource was messing with the ability of the bridge to connect to localstorage, so just include this in the bundle

Copy link
Member

Choose a reason for hiding this comment

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

I recall there are some bad ways to use SVG in React but I don't remember whether this is one of them :) Either way, seems fine for now. Optimize after correctness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in d823ccd, but I'll leave this in anyway because it's small enough and will look better not loading in separately.

@samsiegart samsiegart marked this pull request as ready for review August 17, 2022 19:56
@samsiegart samsiegart requested a review from dckc August 17, 2022 19:59
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

Some suggestions, none blocking

const pathStyle = { fill: '#bb2d40' };

return (
<svg
Copy link
Member

Choose a reason for hiding this comment

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

I recall there are some bad ways to use SVG in React but I don't remember whether this is one of them :) Either way, seems fine for now. Optimize after correctness.

);
const Bridge = () => {
return (
<button className="button" onClick={signalBridge}>
Copy link
Member

Choose a reason for hiding this comment

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

button.button is a funny target. consider an id since there will be exactly one in the DOM:

Suggested change
<button className="button" onClick={signalBridge}>
<button id="signalWallet" onClick={signalBridge}>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to signal-wallet-button

Copy link
Member

Choose a reason for hiding this comment

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

That might have been me, aiming for type=button, which suppresses form submission, iirc

@samsiegart samsiegart added the automerge:squash Automatically squash merge label Aug 18, 2022
@samsiegart samsiegart added automerge:squash Automatically squash merge and removed automerge:squash Automatically squash merge labels Aug 18, 2022
@mergify mergify bot merged commit 94791c9 into master Aug 18, 2022
@mergify mergify bot deleted the pretty-wallet-connection branch August 18, 2022 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants