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

Change UI to be more similar to current extension: App Header #936

Merged
merged 2 commits into from
Jul 22, 2022
Merged

Change UI to be more similar to current extension: App Header #936

merged 2 commits into from
Jul 22, 2022

Conversation

buberdds
Copy link
Contributor

Main requirements were:

  • use account icon generator from extension for Account Dropdown
  • remove search bar (for now I removed it only from mobile, but I am adding screenshots of two versions with and without search for large screens
  • css changes

Screenshot 2022-07-19 at 08 30 50

with search
Screenshot 2022-07-19 at 11 06 13

without search
Screenshot 2022-07-19 at 11 07 13

with dynamic account icon
Screenshot 2022-07-19 at 11 07 58
Screenshot 2022-07-19 at 11 07 51

Screenshot 2022-07-19 at 11 57 10
Screenshot 2022-07-19 at 11 56 45

@github-actions
Copy link

github-actions bot commented Jul 19, 2022 •
edited
Loading

MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 16 0 0.05s
✅ GIT git_diff yes no 0.02s
✅ JSON eslint-plugin-jsonc 5 0 0 1.09s
✅ JSON jsonlint 5 0 1.32s
✅ JSON prettier 5 0 0 0.72s
✅ JSON v8r 5 0 8.94s
✅ TSX eslint 6 0 0 5.93s
✅ TYPESCRIPT eslint 1 0 0 3.65s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

@codecov
Copy link

codecov bot commented Jul 19, 2022 •
edited
Loading

Codecov Report

Merging #936 (a4e1247) into master (f6b85e0) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head a4e1247 differs from pull request most recent head cb61cb9. Consider uploading reports for the commit cb61cb9 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #936      +/-   ##
==========================================
- Coverage   88.04%   88.01%   -0.03%     
==========================================
  Files          98       97       -1     
  Lines        1664     1635      -29     
  Branches      370      362       -8     
==========================================
- Hits         1465     1439      -26     
+ Misses        199      196       -3
Flag Coverage Δ
cypress 58.30% <100.00%> (-0.07%) ⬇️
jest 78.73% <48.27%> (-0.80%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ponents/Toolbar/Features/NetworkSelector/index.tsx 80.00% <ø> (ø)
src/app/components/JazzIcon/index.tsx 100.00% <100.00%> (ø)
src/app/components/Sidebar/index.tsx 88.60% <100.00%> (+0.29%) ⬆️
...s/Toolbar/Features/AccountSelectorButton/index.tsx 100.00% <100.00%> (ø)
src/app/components/Toolbar/index.tsx 100.00% <100.00%> (ø)
src/app/components/AddressBox/index.tsx 66.66% <0.00%> (ø)
src/app/components/AmountFormatter/index.tsx 100.00% <0.00%> (ø)
...ages/AccountPage/Features/AccountSummary/index.tsx 100.00% <0.00%> (ø)

@buberdds buberdds requested review from tjanez and lukaw3d July 19, 2022 12:15
lukaw3d
lukaw3d reviewed Jul 20, 2022
View reviewed changes
src/types/jazzicon.d.ts Outdated Show resolved Hide resolved
src/app/components/Toolbar/Features/AccountSelectorButton/index.tsx Outdated Show resolved Hide resolved
src/app/components/Toolbar/index.tsx Outdated Show resolved Hide resolved
src/app/components/JazzIcon/index.tsx Outdated
Comment on lines 13 to 19
useEffect(() => {
if (ref?.current) {
const icon = jazzicon(diameter, seed)

ref.current.replaceChildren()
ref.current.appendChild(icon)
}
}, [diameter, ref, seed])
Copy link
Member

Choose a reason for hiding this comment

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

Is this the recommended react way to append HTML elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What other option I have to inject DOM elements generated by 3rd party, not react lib? I think it's Ok that's what metamask is doing for example.

Copy link
Member

Choose a reason for hiding this comment

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

Yep yep. I can't find docs for the recommended way 🤷

src/app/components/JazzIcon/index.tsx Outdated Show resolved Hide resolved
@buberdds buberdds force-pushed the buberdds/header branch 2 times, most recently from eaa5ff3 to f442611 Compare July 20, 2022 10:15
tjanez
tjanez approved these changes Jul 20, 2022
View reviewed changes
Copy link
Member

@tjanez tjanez left a comment

Choose a reason for hiding this comment

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

The new icon avatar/switcher looks great!
I would remove the search box completely.

@buberdds buberdds requested a review from lukaw3d July 21, 2022 06:59
@buberdds buberdds merged commit f30f00e into master Jul 22, 2022
@buberdds buberdds deleted the buberdds/header branch July 22, 2022 14:56
@buberdds buberdds mentioned this pull request Jan 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tjanez tjanez tjanez approved these changes

@lukaw3d lukaw3d lukaw3d approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@buberdds @tjanez @lukaw3d