Adding an explicit height of 830px; to HeroCtn in sub-page-hero styles and removing the min height.
The spiningcoin animation was ranging into the navbar, looks cleaner now.
Details
Details
- Reviewers
bytesofman johnkuney - Group Reviewers
Restricted Project - Commits
- rABC4aadac712c85: [e.cash] Add max height to image styles to fix image on what's eCash page
visit e.cash/what-is-eCash, check positioning of spiningcoin animation.
Also check other pages for images being rendered correctly.
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- margin
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 29970 Build 59474: Build Diff Build 59473: arc lint + arc unit
Event Timeline
Comment Actions
Build Bitcoin ABC Diffs / Diff Testing (preview-e.cash) passed.
Preview is available at http://51.83.66.92:41398 for the next 60 minutes.
Comment Actions
Is this specific to some screen size ? On my monitor it just makes the animation appear slightly off center offset to the bottom. I have no issue with the current version also.
Comment Actions
That's how it looks for me and Aimal:
So we tried to putt it further down so that the navigation bar doesn't overlap it:
Comment Actions
imo this is an overcomplicated fix that is likely to cause more problems than it solves. the picture of the original problem -- interference with the nav menu on some devices/resolutions -- does not seem that bad.
if we really want to solve that, we should do it with rules on the nav menu to ensure space below for any given thing, not rules for space above on one particular thing that happens to be below the nav menu in this one case.
Comment Actions
I think the issue is with the subPageHero component.
The min height of the container does not account for the top padding and image height. So tall images are overflowing into the nav and shorter screens.
Just havent had a tall image like this used before I think is why it hasnt been noticed
Adding an explicit height of 830px; to HeroCtn in sub-page-hero styles and removing the min height should fix it
This will also avoid an unnecessarily tall hero section on large monitors
| web/e.cash/components/sub-page-hero/styles.js | ||
|---|---|---|
| 18–19 ↗ | (On Diff #49174) | |
Comment Actions
Okay, some images already have this kind of adjustment on them (e.g.: core tech) , but I agree that the source files need to be corrected. In fact I suggest alternative images to begin with. I will create a new task where I show the alternative imagery we could use and the reason for scrapping these hand-animations altogether. But that is something for a later date I presume.
I will abandon this diff.
Comment Actions
Removed the complex code and added an explicit height for images in styles which will
fix issues with tall images.
Comment Actions
Build Bitcoin ABC Diffs / Diff Testing (preview-e.cash) passed.
Preview is available at http://54.39.104.128:41660 for the next 60 minutes.
Comment Actions
Build Bitcoin ABC Diffs / Diff Testing (preview-e.cash) passed.
Preview is available at http://51.83.66.92:41453 for the next 60 minutes.
Comment Actions
a little wary about hardcoding the height, seems like this would produce different behavior than previous code (which is saying, take up the whole height of the screen, and if the screen is less than 700px tall -- then take up at least 700px).
seems like this could cause some responsiveness issues.
however, I'm not able to find them in testing. something to keep an eye on tho.