-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Feature: Improve TaskPreview UI #20997
Feature: Improve TaskPreview UI #20997
Conversation
@narefyev91 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/Videos |
@Nikhil-Vats Hey! On Web when i use tab button to focus on checkbox i get this |
@Nikhil-Vats also your checklist is failing on github actions - can you please re-check |
@narefyev91 Good catch, I am not sure how it should look. |
Can we make the focus outline go to the outside, and use a similar border radius as the checkbox itself? |
@shawnborton isn't the blue outline repetitive? since we already have the green outline on focus shouldn't we remove the blue one just for checkbox? It looks like this on prod - (the blue is only visible at corner) |
Hm good point. Then perhaps we just get rid of the blue style and use only the green - can you show us what that looks like? |
Yeah, I am kind of thinking we should just use the same blue outline style but make sure it goes to the outside. This way we don't need to use green for focused, because as you pointed out, it competes with the checked state. |
@shawnborton Screenshot 1 - We keep the checkbox vertically aligned with the name. Outline will go to the left. Screenshot 2 - We keep the outline aligned with the name, checkbox will not be vertically aligned with name. Also, since, we need to increase height of checkbox container, gap between TaskPreview (checkbox, assignee, title) and name of user will increase. Top margin added to RightArrow and title/assignee to keep them horizontally aligned with checkbox. See margin in above screenshot. What do you think? |
Hmm I don’t mean for the outline to be that far outside. I mean just a 1px
outline that is right outside of the checkbox border. I feel like a CSS
drop shadow approach would achieve that.
…On Mon, Jun 19, 2023 at 3:14 PM Nikhil Vats ***@***.***> wrote:
@shawnborton <https://github.com/shawnborton>
If we want to add an outline outside of the checkbox, we need to increase
the size of its container which leads to following two scenarios -
Screenshot 1 - We keep the checkbox vertically aligned with the name.
Outline will go to the left.
[image: Screenshot 2023-06-20 at 12 34 23 AM]
<https://user-images.githubusercontent.com/31413407/246913716-0d28910d-b035-4a11-8a3d-f75980891316.png>
Screenshot 2 - We keep the outline aligned with the name, checkbox will
not be vertically aligned with name.
[image: Screenshot 2023-06-20 at 12 32 47 AM]
<https://user-images.githubusercontent.com/31413407/246913486-94f7e761-6c88-4d16-9652-8e7765c2a7cd.png>
Also, since, we need to increase height of checkbox container, gap between
TaskPreview (checkbox, assignee, title) and name of user will increase. Top
margin added to RightArrow and title/assignee to keep them horizontally
aligned with checkbox. See margin in above screenshot.
What do you think?
—
Reply to this email directly, view it on GitHub
<#20997 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AARWH5WPTIWX2M6AG26FHR3XMCQHRANCNFSM6AAAAAAZLEMW6Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@shawnborton This is when there is a small gap between the outline and checkbox to make sure it is properly visible for checked style - |
I like the second option here! |
@narefyev91 PR updated as per @shawnborton's suggestions. In TaskPreview - new.outline.movIn other components - outline.desktop.app.mov |
Nice, thank you! |
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.
LGTM
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.
Great, thanks!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/thienlnam in version: 1.3.30-0 🚀
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.3.30-5 🚀
|
@@ -2453,13 +2453,20 @@ const styles = { | |||
alignItems: 'center', | |||
}, | |||
|
|||
checkboxPressable: { | |||
borderRadius: 6, | |||
padding: 2, |
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.
✋ Coming from #27703
The padding here makes the checkbox a few pixels far to the right and does not align with the left line of other inputs. More details #27703 (comment)
Details
Improve the UI of TaskPreview by changing outline of checkbox and anchoring it to top.
Fixed Issues
$ #20535
$ #20535 (comment)
Tests
Updated styles should look like this. Click to expand.
Offline tests
Same as above.
QA Steps
Same as above.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
20535_chrome.mov
20535_chrome_2.mov
Mobile Web - Chrome
20535_mWeb.android.mov
Mobile Web - Safari
20535_mweb_ios.mov
Desktop
20535_desktop.mov
iOS
20535_ios.mov
Android
20535_android.mov