-
Notifications
You must be signed in to change notification settings - Fork 984
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
fix: connectivity banner height #10365
Conversation
Hey @emizzle, and thank you so much for making your first pull request in status-react! ❤️ Please help us make your experience better by filling out this brief questionnaire https://goo.gl/forms/uWqNcVpVz7OIopXg2 |
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (18)
|
hey @emizzle great job! could you please remove all commented lines, and revert back all formatting, thanks |
@flexsurfer - comments removed. I've asked in #desktop discord if we have a standardised |
5023bf9
to
2e55ef2
Compare
[react/text {:style styles/text | ||
:on-press (when on-press-event #(re-frame/dispatch [on-press-event]))} | ||
(i18n/label message)]))]))}))) | ||
:component-did-update (fn [comp] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but cljfmt doesn't do that, we try to keep the length of the lines under 80 chars (soft limit), for this reason in cases like this we put the key and value on separate lines
25c1168
to
279dd93
Compare
98% of end-end tests have passed
Failed tests (2)Click to expand
Passed tests (89)Click to expand |
279dd93
to
718ec7f
Compare
Failed e2e tests are not related, they are already fixed in develop |
718ec7f
to
0f6c2e0
Compare
Well, I couldn't trigger 2-lines connectivity banner, but 1-lines looks OK to me on Android and IOS. |
0f6c2e0
to
4b97699
Compare
Fixes #9803 --the height of the banner was cutting off banner messages spanning multiple lines Due to the animation of the view surrounding the banner text, calculate the banner height, then re-render the banner with the updated height. Render a view that wraps not only the message banner itself, but also a hidden (0-opacity and absolutely positioned) view with the banner text that calculates the height of the banner during the `on-layout` event. The banner height calculation causes the reagent atom value change to re-render the banner view with the udpated height. 1. There is an animation on the message banner. The "out" animation (ie when the banner is in the process of disappearing) seems to work correctly. However, there did not seem to be an "in" animation. I'm fairly confident these changes did not break any existing animations, however it's worth noting this so that others who are more familiar with the existing codebase may be in a better place to say otherwise. 2. Testing during changes was done by dispatching the `network-info-changed` event like so: ``` (re-frame/dispatch [::network-info-changed {:isConnected false}]) ``` Co-authored-by: Pascal Precht <pascal@status.im> Co-authored-by: Eric Dvorsak <eric@status.im>
4b97699
to
5b7fad0
Compare
merged |
Fixes #9803
Summary
The height of the banner was cutting off banner messages spanning multiple lines
Fix
Due to the animation of the view surrounding the banner text, calculate the banner height, then re-render the banner with the updated height.
Details
Render a view that wraps not only the message banner itself, but also a hidden (0-opacity and absolutely positioned) view with the banner text that calculates the height of the banner during the
on-layout
event. The banner height calculation causes the reagent atom value change to re-render the banner view with the udpated height.Review notes
Areas that maybe impacted
Any chat screen (chat list, 1-on-1, chat group, etc).
Steps to test
network-info-changed
event like so:1586
insrc/status_im/subs.cljs
to add a long message, ie:t/mailserver-request-error-status
can be used.status: ready
Co-authored-by: Pascal Precht pascal@status.im
Co-authored-by: Eric Dvorsak eric@status.im