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

alcipir created this revision.Dec 1 2020, 03:12

Owners added a reviewer: Restricted Owners Package.Dec 1 2020, 03:12

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 1 2020, 03:12

alcipir requested review of this revision.Dec 1 2020, 03:12

Harbormaster completed remote builds in B14250: Diff 26104.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.

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:

Test plan for cashtab changes will probably always be npm install && npm start followed by npm test
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.

Harbormaster completed remote builds in B14264: Diff 26122.Dec 1 2020, 15:32

alcipir edited the summary of this revision. (Show Details)Dec 1 2020, 15:33

alcipir edited the test plan for this revision. (Show Details)

Please use fbt. Our other stuff use fbt.

This revision now requires changes to proceed.Dec 1 2020, 15:58

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

Harbormaster completed remote builds in B14271: Diff 26144.Dec 2 2020, 07:22

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

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

Harbormaster completed remote builds in B14279: Diff 26155.Dec 2 2020, 22:07

alcipir added inline comments.Dec 2 2020, 22:08

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.