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

feat: Add AddressDisplayer for mobile, closes LEA-1638 #603

Closed
wants to merge 6 commits into from

Conversation

tigranpetrossian
Copy link
Contributor

Adds native version of AddressDisplayer, adds Fira Code and addresses some minor font/styling issues as a prerequisite:

  1. Added Fira Code for mobile, new and correct weights for the web (had to compile woff2 and otf myself from source, as they weren't present in the official build 😬), updated references everywhere.
  2. Update storybook for native, update the build to make sure all fonts are displayed correctly.
  3. Add address textStyle to tokens, as code that's currently used for the web version of the AddressDisplayer doesn't match the new designs. I'm not extremely happy with the text style name this specific, brought this up with the design team to see if it's worth using something more generic.
  4. Refactored the AddressDisplayer for web to match the current designs:
    1. Used the new text style
    2. Added a flexbox wrapper to correct line height (didn't work with inline context), replaced margin with 1ch wide gaps to avoid trailing spaces.
    3. Had to merge into a single component, the layout pattern didn't make sense after doing the above, thankfully this is a fairly small component.
  5. Added native version of the component

@edgarkhanzadian I would really appreciate your help double checking the config and all the artifacts expo generated after running prebuild to bundle fonts. I'm way out of my depth when it comes to this kind of thing.

Copy link

linear bot commented Nov 6, 2024

@tigranpetrossian
Copy link
Contributor Author

@kyranjamie Missed an important part: extension will likely need a small PR to address the changes to the component here. Two questions:

  1. Is there script that pulls the base.css and font files from the UI package automatically, or is it done manually?
  2. How do we typically orchestrate releases when there's a potentially breaking change in the monorepo that affects the extension?

@kyranjamie
Copy link
Collaborator

Is there script that pulls the base.css and font files from the UI package automatically, or is it done manually?

No, and I didn't realise they were copied. We shouldn't have duplicates of this file, ideally the base.css in the extension reads this one in the mono.

How do we typically orchestrate releases when there's a potentially breaking change in the monorepo that affects the extension?

This is a mostly manual process currently. Suggest linking the packages locally, making a PR in the extension (with a broken build because the package hasn't been released yet), then updating the PR when you've released the monorepo changes.

Copy link

codecov bot commented Nov 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 24.23%. Comparing base (58fc169) to head (7acca95).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev     #603   +/-   ##
=======================================
  Coverage   24.23%   24.23%           
=======================================
  Files         155      155           
  Lines        5847     5847           
  Branches      324      324           
=======================================
  Hits         1417     1417           
  Misses       4430     4430           
Components Coverage Δ
bitcoin 62.04% <ø> (ø)
query 12.49% <ø> (ø)
utils 48.31% <ø> (ø)
crypto 68.21% <ø> (ø)
stacks 54.71% <ø> (ø)

Copy link
Contributor

@pete-watters pete-watters left a comment

Choose a reason for hiding this comment

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

Nice work here @tigranpetrossian , this is a really clean and valuable PR 🚀

If you run pnpm syncpack:format it should fix your lint error

@pete-watters
Copy link
Contributor

@kyranjamie Missed an important part: extension will likely need a small PR to address the changes to the component here. Two questions:

  1. Is there script that pulls the base.css and font files from the UI package automatically, or is it done manually?
  2. How do we typically orchestrate releases when there's a potentially breaking change in the monorepo that affects the extension?

When originally building the UI package we decided against hosting the font files and static assets in there and instead keeping them local to the extension. That's the reason for this base.css duplication.

It could be better to remove that duplication and keep everything in the UI library but we would need to do some work on bundling the font files for export with the UI package. We also have other static assets in extension/public anyway so I think it's OK to have small duplication here to avoid more complication.

@tigranpetrossian
Copy link
Contributor Author

Nice work here @tigranpetrossian , this is a really clean and valuable PR 🚀

If you run pnpm syncpack:format it should fix your lint error

Thanks! Did this and it seems fine. I suspect the errors are either because I either modified the history of the branch too much or we need to add a pnpm install prior to running these scripts. Will investigate.

@tigranpetrossian
Copy link
Contributor Author

tigranpetrossian commented Nov 7, 2024

When originally building the UI package we decided against hosting the font files and static assets in there and instead keeping them local to the extension. That's the reason for this base.css duplication.

It could be better to remove that duplication and keep everything in the UI library but we would need to do some work on bundling the font files for export with the UI package. We also have other static assets in extension/public anyway so I think it's OK to have small duplication here to avoid more complication.

@pete-watters Agreed. I'm looking into this. If it turns out to be too complicated we'll tackle it separately or worst case scenario stick with duplication.

Copy link
Collaborator

@edgarkhanzadian edgarkhanzadian left a comment

Choose a reason for hiding this comment

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

Looks amazing ! 🚀

I've flagged a few more lines that prebuild has automatically added to the ios folder.

apps/mobile/ios/leatherwalletmobile/Info.plist Outdated Show resolved Hide resolved
apps/mobile/ios/leatherwalletmobile/Info.plist Outdated Show resolved Hide resolved
@tigranpetrossian tigranpetrossian force-pushed the feat/address-displayer-native branch 2 times, most recently from 8e68d02 to 4beca3b Compare November 7, 2024 10:03
Copy link
Contributor

@fbwoolf fbwoolf left a comment

Choose a reason for hiding this comment

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

Lgtm @tigranpetrossian 👍

@tigranpetrossian tigranpetrossian added this pull request to the merge queue Nov 7, 2024
@camerow camerow removed this pull request from the merge queue due to a manual request Nov 7, 2024
@camerow camerow added this pull request to the merge queue Nov 7, 2024
@camerow
Copy link
Collaborator

camerow commented Nov 7, 2024

I saw this was sitting in the merge queue for ~6 hours. Not sure what is going on but tried removing/readding it.

@camerow camerow removed this pull request from the merge queue due to the queue being cleared Nov 7, 2024
@camerow camerow added this pull request to the merge queue Nov 7, 2024
@fbwoolf fbwoolf removed this pull request from the merge queue due to the queue being cleared Nov 8, 2024
@fbwoolf fbwoolf added this pull request to the merge queue Nov 8, 2024
@fbwoolf
Copy link
Contributor

fbwoolf commented Nov 8, 2024

Trying again.

@tigranpetrossian tigranpetrossian removed this pull request from the merge queue due to a manual request Nov 8, 2024
@tigranpetrossian tigranpetrossian added this pull request to the merge queue Nov 8, 2024
@tigranpetrossian tigranpetrossian removed this pull request from the merge queue due to a manual request Nov 8, 2024
@tigranpetrossian tigranpetrossian added this pull request to the merge queue Nov 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a manual request Nov 8, 2024
@tigranpetrossian tigranpetrossian deleted the feat/address-displayer-native branch November 8, 2024 07:20
@tigranpetrossian tigranpetrossian restored the feat/address-displayer-native branch November 8, 2024 07:21
@edgarkhanzadian edgarkhanzadian deleted the feat/address-displayer-native branch November 8, 2024 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants