-
Notifications
You must be signed in to change notification settings - Fork 24
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
Properties panel design finishing #105
Conversation
07e64d7
to
0736a8c
Compare
0736a8c
to
a8fb522
Compare
@@ -62,7 +62,8 @@ export default function Group(props) { | |||
return <div class="bio-properties-panel-group" data-group-id={ 'group-' + id }> | |||
<div class={ classnames( | |||
'bio-properties-panel-group-header', | |||
edited ? '' : 'empty' | |||
edited ? '' : 'empty', | |||
open? 'open' : '' |
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.
open? 'open' : '' | |
open ? 'open' : '' |
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.
So that it doesn't look like similar to optional chaining.
Shall we also add a visual indicator for the checkbox focus? checkbox.mp4 |
<button class="bio-properties-panel-group-header-button"> | ||
<GroupArrowIcon class={ open ? 'bio-properties-panel-arrow-down' : 'bio-properties-panel-arrow-right' } /> | ||
<button | ||
title="Toggle section" |
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.
Did we agree to call this section instead of group?
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.
Not sure whether we ever actively agree on a specific term here, but "section" is part of our technical writing guide.
"Group" seems to be something we came up technically.
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.
👍🏻
@@ -148,7 +148,10 @@ export default function ListGroup(props) { | |||
AddContainer | |||
? ( | |||
<AddContainer> | |||
<button class="bio-properties-panel-group-header-button bio-properties-panel-add-entry"> | |||
<button | |||
title="Create new list item" |
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 title is very generic. Are we okay with that or should we make this configurable (e.g. Create new input)?
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.
It's part of our design prototype, so it might be ok to keep it generic. We can update it afterward if we see a need though.
/cc @andreasgeier
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.
LGTM
For the future, I think it would be cool to split such loaded PRs to several smaller ones. |
Re-requesting my review so that you can continue discussion with Flip. |
Damn, I'm on the bpmn-properties-panel UX branch. 🤦🏻 |
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.
👏🏻
Related to bpmn-io/bpmn-properties-panel#90
Closes #15
Design updates for the properties panel (cf. bpmn-io/bpmn-properties-panel#90)
This works alongside bpmn-io/bpmn-properties-panel#128, which can be tested via command
npx @bpmn-io/sr bpmn-io/bpmn-properties-panel#90-ui-ux-improvements -l bpmn-io/properties-panel#ux-ui-improvements -c 'npm run start:platform'