-
Notifications
You must be signed in to change notification settings - Fork 985
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 small UI problems with the compose reply screen #8959
Conversation
Hey @alexjg, 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 (49)
|
thanks @alexjg • The reply icon is displayed at incorrect size (I'm guessing 20), it should be 16 by 16 |
@errorists That asset looks like it's not been added to the app (that is, I can't see a |
it's cool, I've exported it here. As far as I can tell, we use PNGs, at 1x,2x,3x densities and tint it to get the correct colour. Alternatively, you can try and sample the close icon located under the Browser tab. It might be that it's made with a circular grey view and 'X' icon added on top, not sure, best to ask @flexsurfer |
yes it's just a view with the icon, you can find it here https://github.com/status-im/status-react/blob/d07c9c6613551a6b68ff1087276f08aa64f61a71/src/status_im/ui/screens/browser/open_dapp/views.cljs#L49 |
looks good @alexjg ! |
@alexjg could you please rebase onto develop and squash commits, thanks |
3fe56e4
to
88f971f
Compare
Done |
@alexjg linter is complaining about your recent changes, you can use |
88f971f
to
07fd799
Compare
Done |
96% of end-end tests have passed
Failed tests (2)Click to expand
Passed tests (44)Click to expand |
@errorists ios ready to test |
@flexsurfer @errorists wasn't it tested already? can we merge? |
In the testing I just did, the reply icon is not displayed at all. Other than that I couldn’t find any issues |
Hey @alexjg are you still planning on finishing this PR? |
@yenda yep, I've been on holiday for the past few days, hence slow replies. I'll squash once I'm home tomorrow. |
aa7ce81
to
ee8e780
Compare
@yenda I've squashed and pushed. One thing I've noticed is that the recent gfycat changes mean that the address of the account you're responding to appears rather than the gfycat of it. I'm not sure if this is a bug or not and it definitely seems unrelated to the functionality implemented here but I thought I would mention it. Everything should be hunky dory for merge now. |
@yenda can you review it again? |
@yenda |
89% of end-end tests have passed
Failed tests (5)Click to expand
Passed tests (42)Click to expand |
ee8e780
to
4a2a2db
Compare
Is there anything I can do to move this forward? Or is it a case of waiting for a fix for the identicons issue which is being worked on elsewhere? I'm happy to have a look at the identicons issue myself if no one is doing so already. |
@alexjg can you include this small change in your PR https://github.com/status-im/status-react/pull/9076/files ? It is not the right fix because the alias will show as a username but at least it won't show a public key instead of the name |
4a2a2db
to
dccdccd
Compare
@yenda done |
:else | ||
[react/text {:style {:color colors/gray :font-size 12 :font-weight "400"}} | ||
from])) | ||
(let [additional-styles (style false)] |
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.
sorry one last thing are you sure this is actually necessary? what is this argument for? I can't find anywhere where it is true, I'm wondering if we couldn't remove the whole chosen?
param in these styles fns
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.
Yeah it doesn't seem like it's used anywhere but I wasn't completely confident of that (given my inexperience with the codebase) so I made the smallest change I could. It certainly does seem like we could remove the chosen?
parameter, want me to go ahead and do that?
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.
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.
I'm out this week ill let @flexsurfer decide
@yenda is there anything remaining to be done to get this merged? I'm keen to get it done as the gitcoin bounty attached to it expires in 2 days. |
dccdccd
to
bdc6152
Compare
I've rebased from |
Tested on IOS 11.4.1. and Android 8 (LG V20):
From my POV can be merged. |
bdc6152
to
8ba3920
Compare
Signed-off-by: Andrey Shovkoplyas <motor4ik@gmail.com>
8ba3920
to
78c57a3
Compare
Fixes #7781
Summary
Implements the small design tweaks in the linked issue
status: ready
I've attached a screenshot of this running on a Pixel 2. I haven't tested on an iDevice.