-
Notifications
You must be signed in to change notification settings - Fork 893
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
UX-2323 Update existing UI Guidelines: the Checkbox guideline update, Eldar Isiangulov #1391
base: main
Are you sure you want to change the base?
Conversation
… Eldar Isiangulov
… Eldar Isiangulov
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.
Also left a couple comments in the Figma file
<tldr> | ||
<p> | ||
<control>Implementation:</control> | ||
<a href="https://docs.oracle.com/javase/tutorial/uiswing/components/button.html"><code>JCheckBox</code></a>, |
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.
Why button?
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.
The guide there refers to buttons, checkboxes and radio buttons — everything on one page. Also, I've checked it and couldn't find a way to paste an anchor to the checkbox chapter
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.
may be add this link? https://docs.oracle.com/javase/8/docs/api/javax/swing/JCheckBox.html
topics/ui/controls/checkbox.topic
Outdated
</tr> | ||
</table> | ||
<tabs group="languages"> | ||
<tab title="Kotlin UI DSL" group-key="kotlin"> |
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.
These code snippets are for the Long labels chapter, please move under it
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, I've moved the code snippets to the correct place
topics/ui/controls/checkbox.topic
Outdated
</tr> | ||
<tr> | ||
<td width="378"> | ||
<img src="checkbox_when_not_to_use_3.png" alt="Two radio buttons with clear labels"/> |
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 think, it's not as informative without the incorrect example. There was an example in the old article
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.
Have reworked it into two separate tables with incorrect and correct examples
row:</p> | ||
<img src="checkbox_when_to_use_3.png" | ||
alt="A table with checkboxes where the label is placed into the column header" width="706"/> | ||
<chapter title="Implementation" collapsible="true" id="checkbox-table-implementation"> |
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.
Alignment of the "Implementation" section seems off, added screenshot to the Figma file under the checkboxes pics
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, I think we should discuss it separately if the placement of the "+" buttons in collapsible sections can be changed
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.
Let's work on all layout issues separately, since it should be fixed on the wrs frontend side but not by us in the help sources. I suggest that we may gather a long list of improvements as we work on guidelines
topics/ui/controls/checkbox.topic
Outdated
<p>In a group of options, use the parent checkbox to show the status of its children:</p> | ||
<img src="checkbox_three_state.png" | ||
alt="Different states for a parent checkbox: checked, indeterminate, and unchecked" width="706"/> | ||
<p>The parent checkbox in checked, indeterminate and unchecked states.</p> |
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.
In the old article this was a caption for the illustration, see https://jetbrains.github.io/ui/controls/checkbox/. I suggest making it a second sentence above the pic:
"In a group of options, use the parent checkbox to show the status of its children. The parent checkbox in checked, indeterminate and unchecked states:"
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.
Good idea, thanks!
topics/ui/controls/checkbox.topic
Outdated
class which represents its state with the <code>ThreeStateCheckBox.State</code> enum containing <code>SELECTED, | ||
NOT_SELECTED, DONT_CARE</code> states.</chapter> | ||
<p>When the user clicks an indeterminate checkbox for the first time, the whole group becomes checked. The | ||
second click unchecks the whole group.</p> |
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.
Possibly also show this paragraph above the pic? So that the pic separates two different usages of the indeterminate checkbox
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.
Will do, thanks!
topics/ui/controls/checkbox.topic
Outdated
Repositories "tools-base" and "contrib" are being loaded. When loading is | ||
finished, the indeterminate checkbox will be replaced with the checked checkbox if there are | ||
commits, or an unchecked checkbox if there are no commits. | ||
</p> |
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.
This was also an image caption in the old article. I'd suggest moving above the image
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.
Done
topics/ui/controls/checkbox.topic
Outdated
</chapter> | ||
<chapter title="Writing guidelines" id="writing-guidelines"> | ||
<list> | ||
<li>Use sentence-style capitalization: |
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.
Bullet points seem a bit noisy. Illustrations already work as separators between rules, possibly could be ok without bullet points?
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 agree, I removed them
topics/ui/controls/checkbox.topic
Outdated
</list> | ||
</chapter> | ||
</chapter> | ||
<chapter title="Placement" id="placement"> |
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.
Possibly rename to "How to layout" and give link to https://plugins.jetbrains.com/docs/intellij/layout.html#checkboxes-and-radio-buttons
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.
Thanks, I agree! Plus, it will be just one link and the chapter would cover everything
…, Eldar Isiangulov
…, Eldar Isiangulov
…, Eldar Isiangulov
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, just left some minor comments
</chapter> | ||
<chapter title="How to layout" id="how-to-layout"> | ||
<p> | ||
Follow the <a href="layout.md" anchor="checkboxes-and-radio-buttons">layout of checkboxes and radio buttons</a> |
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.
period is missing
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.
Or even rephrase: Follow the guidelines for checkoxes and radio buttons layout.
row:</p> | ||
<img src="checkbox_when_to_use_3.png" | ||
alt="A table with checkboxes where the label is placed into the column header" width="706"/> | ||
<chapter title="Implementation" collapsible="true" id="checkbox-table-implementation"> |
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.
Let's work on all layout issues separately, since it should be fixed on the wrs frontend side but not by us in the help sources. I suggest that we may gather a long list of improvements as we work on guidelines
No description provided.