Page MenuHomePhabricator

[CashTab] Add Fbt dependencies, init script and intl to App component
ClosedPublic

Authored by alcipir on Dec 1 2020, 03:12.

Details

Reviewers
bytesofman
deadalnix
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABCf0a9f67f8329: [CashTab] Add Fbt dependencies, init script and intl to App component
Summary

See title.

Test Plan

npm install & npm start. Followed by npm run test.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
transifex-cashtab
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 14264
Build 28507: Build Diff
Build 28506: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Dec 1 2020, 03:12
alcipir requested review of this revision.Dec 1 2020, 03:12
alcipir retitled this revision from Add Transifex dependencies and basic setup to Add Transifex dependencies and basic setup to CashTab.Dec 1 2020, 03:14
alcipir edited the test plan for this revision. (Show Details)
alcipir added a reviewer: bytesofman.
alcipir retitled this revision from Add Transifex dependencies and basic setup to CashTab to [CashTab] Add Transifex dependencies and basic setup to CashTab.
bytesofman requested changes to this revision.Dec 1 2020, 14:28

This certainly makes for a small diff since there are no code changes beyond installing npm packages. I think, though, that this makes it irrelevant, and also introduces this linting error:

Compiled with warnings.

./src/index.js
  Line 9:10:  'T' is defined but never used  no-unused-vars

I don't think we gain anything here; revoking this diff would only remove dependencies and not any code.

Going forward:

  1. Test plan for cashtab changes will probably always be npm install && npm start followed by npm test
  2. Code change should involve some behavior change, otherwise if we need to revoke the behavior-change diff, we might need to dig this one up too
This revision now requires changes to proceed.Dec 1 2020, 14:28

Adds changes to App component to handle Transifex strings.

alcipir edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Dec 1 2020, 15:58
deadalnix added a subscriber: deadalnix.

Please use fbt. Our other stuff use fbt.

This revision now requires changes to proceed.Dec 1 2020, 15:58
alcipir edited the test plan for this revision. (Show Details)

Adds fbt dependencies and init script, adds intl on App component (English/Portuguese)

alcipir retitled this revision from [CashTab] Add Transifex dependencies and basic setup to CashTab to [CashTab] Add Fbt dependencies, init script and intl to App component.Dec 2 2020, 07:23
alcipir edited the summary of this revision. (Show Details)
alcipir edited the test plan for this revision. (Show Details)
deadalnix requested changes to this revision.Dec 2 2020, 15:29
deadalnix added inline comments.
web/cashtab/package-lock.json
44 ↗(On Diff #26144)

I'm not convinced that it will help you. You will have to do some manual wiring with transifex.

web/cashtab/translations/en_US.json
26 ↗(On Diff #26144)

This is fine for now, but needs to eventually be sourced from transifex via scripts.

This revision now requires changes to proceed.Dec 2 2020, 15:29
alcipir marked an inline comment as done.
alcipir edited the test plan for this revision. (Show Details)

Removed fbt-easy-setup, that wasn't being used

web/cashtab/package-lock.json
44 ↗(On Diff #26144)

Yes, it is also not maintained anymore, I removed it.

deadalnix requested changes to this revision.Dec 2 2020, 23:47
deadalnix added inline comments.
web/cashtab/src/components/App.js
220 ↗(On Diff #26155)

The description is meant for translator, so that they get context. Adding text they already have is not helpful.

web/cashtab/translation_input.json
1 ↗(On Diff #26155)

This is autogenerated, right? I don't think this should be committed.

This revision now requires changes to proceed.Dec 2 2020, 23:47

Fix description for strings on App component
Fix .gitignore

This revision is now accepted and ready to land.Dec 4 2020, 04:29