Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABC8c05d635fa04: [Cashtab] Fixate header

Summary

As per feedback from D15188, this diff fixates the header in order to better facilitate tx history pagination in D15188.

Test Plan

npm test
npm start
click on a few txs to expand their contains and take up extra vertical space, scroll up and down and ensure the header and footers are fixed
npm run build, or load d15277.netlify.app on ios and android mobile devices and ensure header is fixed while scrolling history
npm run extension and ensure the same fixed header behavior

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

emack created this revision.Jan 25 2024, 03:26

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 25 2024, 03:26

Harbormaster completed remote builds in B26642: Diff 44571.Jan 25 2024, 03:29

emack edited the test plan for this revision. (Show Details)Jan 25 2024, 07:38

Changed header position from fixed to sticky

emack published this revision for review.Jan 25 2024, 07:42

emack added a reviewer: bytesofman.

Harbormaster completed remote builds in B26644: Diff 44573.Jan 25 2024, 07:44

bytesofman added inline comments.Jan 25 2024, 14:49

cashtab/src/components/App.js
185–199 ↗	(On Diff #44573)	do we really need all of these props? `z-index:2` -> why? `height: 180px` -> how does this behave when there is an API error in fiat price? Right now this reduces the height of BalanceHeader `border-top` -> cashtab does not have this currently, why the addition?

emack added inline comments.Jan 26 2024, 02:08

cashtab/src/components/App.js
185–199 ↗	(On Diff #44573)	`z-index:2` -> so it's on the same layer as the tx history and footer, otherwise the tx history will scroll in front of the header `height: 180px` -> you're right, having this height does not reduce the overall header on fiat price error Hence I've removed it and it now dynamically reduces the balance height on api error `border-top` -> yup this can go, removed. Retested on mobile and extension, all good. (d15277.netlify.app also updated)