Skip to content
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

built-in-button #1389

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

built-in-button #1389

wants to merge 6 commits into from

Conversation

MariannaKononenko
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@karollewandowski karollewandowski Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Light and dark mode screenshots show different states.
Is it expected?

@@ -350,7 +350,7 @@
</table>
</chapter>
<chapter title="Cancel" id="cancel">
<table style="none" column-width="fixed">
<table style="none" border="false" column-width="fixed">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please stick to the same order in elements? It will be easier to scan if any edits are needed.
I mean that the border attribute in lines 316, 335, 389 are last, but in 353 and 416 are not last.

@karollewandowski
Copy link
Collaborator

I tried to test it locally, and borders are still present. I updated the wrs-supernova to the latest. Is it supposed to work?

@MariannaKononenko MariannaKononenko requested review from OlyaB and removed request for eldar-jetbrains December 20, 2024 11:00

### Browse
The **Browse** button shows a dialog for opening files from the disk, a tree view, or a table of values.
Use the **Browse** icon to select a single file or a folder from the disc.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Let's remove this part: ", a tree view, or a table of values". I don't remember why we have this. For a tree or table, we have the "List values" case below.
  2. Seems like the second sentence has very similar meaning to the first, if the part from my point 1 is removed. Possibly these parts could be combined?


### List values

Use a control with the table icon to select from the list of classes, methods or environment variables:
Use the **List** icon to select a value from the list of classes, methods, or environment variables:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make it not just classes, methods, and variables, but any object except for files from disc. Classes, methods and variables could be just an example.


If the input text can be long and place is constrained, use a built-in button to expand the control
If the input text can be long and the place is constrained,
use the **Expand** button to expand the control
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly rewrite the second part of the sentence as "...use an input field with the Expand button". Otherwise, the sentence logic is a bit broken.


Do **not** use the Show Viewer button instead.
When the field is expanded, show the **Collapse** button:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the built-in behavior for this control, it will show the Collapse button itself. Possibly just merge this illustration with the previous one, show both default and expanded states nearby, and remove this sentence?


### Copy, Info
To place a button inside a text field, use [`ExtendableTextField`](%gh-ic%/platform/platform-api/src/com/intellij/ui/components/fields/ExtendableTextField.java) and
its `addExtension()` method.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly this part should be in the "Implementation" box at the top like it is for the Split Button?

To place a button inside a text field, use [`ExtendableTextField`](%gh-ic%/platform/platform-api/src/com/intellij/ui/components/fields/ExtendableTextField.java) and
its `addExtension()` method.

The shortcut for a built-in button is <shortcut>Shift+Enter</shortcut>.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly add: 1) a subheader "Shortcut", 2) that the shortcut activates only when the input field with the button is focused.

The only difference is that the selected value is added, instead of overwriting the existing one.
Place the plus icon inside the control.
![](plus.png){width=250}
Place the built-in button inside the input field. Do not place the built-in button next to the field control:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The button outside a control is a regular button, not a built-in button. The purpose of the built-in button is to replace this external button and reduce the number of different controls in a dialog, make it more ordered and simple. Possibly move this part under the "When to use" header? This is like a general guideline when to use the built-in button at all — when there's an action to browse, expand, etc. for an input field or a combo box, then don't use a regular button, use a built-in button.

<table style="none" border="false">
<tr>
<td><format color="369650" style="bold">Correct</format>
<img src="built_in_button_browse_correct.png" alt="Browse button" width="378"/></td>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the correct example, possibly add a second field with the "list" icon? It would work as a correct example for the button with "..." in the incorrect example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants