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

Focus indicators consistent #4728

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Focus indicators consistent #4728

wants to merge 8 commits into from

Conversation

abdul99ahad
Copy link
Contributor

@abdul99ahad abdul99ahad commented Nov 26, 2024

Proposed Changes

Tab action buttons

Earlier: no focus
Now:
image

Checkbox

Earlier:
image

Now:
image

Buttons

image

Changes:
image

Bottom panel tab bar

Earlier: no focus
Changes:
image (changes required)

Bottom panel icons

Earlier: no focus
Changes:
image

Bottom panel focus alignment issue

Earlier: image
Changes:
image

Overlay buttons

Earlier: image

Changes:
image

Footer icons

Earlier: grey background
image

Changes:
image

Variable outline tab

Earlier: no focus
Changes:
image

Checklist

To ensure you provided everything we need to look at your PR:

  • Brief textual description of the changes present
  • Visual demo attached
  • Steps to try out present, i.e. using the @bpmn-io/sr tool
  • Related issue linked via Closes {LINK_TO_ISSUE} or Related to {LINK_TO_ISSUE}

@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Nov 26, 2024
@abdul99ahad abdul99ahad marked this pull request as draft November 26, 2024 09:23
@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed needs review Review pending labels Nov 26, 2024
Copy link

This Pull Request targets develop branch, but contains fix commits.

Consider targeting main instead.

@@ -24,6 +24,18 @@
font-size: 14px !important;
}
}

.bio-vo-text-button {
Copy link
Member

Choose a reason for hiding this comment

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

Is this about the specific variable outline or a general Carbon customization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is general carbon customization. Nevertheless, we have to see in variable-outline project.

@@ -21,4 +21,15 @@
--font-family-monospace: inherit;
background: var(--color-white);
}

.bio-properties-panel-input[type="checkbox"]:focus {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a general properties panel fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fix is implemented in bpmn-io/properties-panel#390

Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

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

Good progress, some early comments:

  • Cannot be focused yet:
    image

  • I see two versions of the outline (thin, and thick), do we plan to align this?
    image
    image

  • I see two versions of outline styles now for buttons, do we plan to align this?
    image
    image

  • Collapsed properties panel is still part of focusable (tabbable) application; we should detach it once collapsed:

    capture 4H6aN7_optimized

Generally I'd suggest we fix the outlines in the relevant libraries, where they are broken, and don't monkey patch in modeler. Applies to properties panel and variable outline.

@abdul99ahad
Copy link
Contributor Author

image

Improved the tab bar icon focus. The styles are consistent and I think the difference is very minor if you look closely. Do we invest more time to align this more perfectly or is it good to have?

cc: @nikku @lmbateman

@abdul99ahad abdul99ahad force-pushed the focus-indicators branch 3 times, most recently from 8b69b40 to 48a989b Compare November 26, 2024 15:48
@abdul99ahad
Copy link
Contributor Author

image Improved the tab bar icon focus. The styles are consistent and I think the difference is very minor if you look closely. Do we invest more time to align this more perfectly or is it good to have?

cc: @nikku @lmbateman

I think it's much better now.
image

@abdul99ahad abdul99ahad marked this pull request as ready for review November 27, 2024 09:49
@bpmn-io-tasks bpmn-io-tasks bot added the needs review Review pending label Nov 27, 2024
@bpmn-io-tasks bpmn-io-tasks bot removed the in progress Currently worked on label Nov 27, 2024
@abdul99ahad
Copy link
Contributor Author

abdul99ahad commented Nov 27, 2024

Summary of the changes:

  • Tabs and tab action buttons have focus
  • Collapsed panel elements won't get focus
  • Bottom bar have same consistent focus states
  • Adjusted focus states offset for certain elements (buttons, containers, etc.,)
  • Properties panel focus states are fixed here
  • Variable outline focus states are fixed here

Figma designs can be found here

@lmbateman
Copy link

lmbateman commented Nov 29, 2024

Collapsed properties panel is still part of focusable (tabbable) application; we should detach it once collapsed (Nico, above)
resolved collapsed panel to be not part of focus anymore (Abdul, in Slack)

I'm a little confused. Does this mean that

  • when the Properties panel is collapsed, it completely drops out of the tab sequence, or
  • when the Properties panel is collapsed, focus lands once on the panel as a whole, but not the items inside the panel?

ETA: The second one is great. I think the first one would make it impossible to open the Properties panel with the Tab and Return keys.

@nikku
Copy link
Member

nikku commented Nov 29, 2024

when the Properties panel is collapsed, focus lands once on the panel as a whole, but not the items inside the panel?

This, "the second one", is what I'd like to see:

  • You can focus the "expand trigger", to expand the properties panel (using keyboard tabbing)
  • But, if the panel is closed, then you're not able to focus items within it.

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

Successfully merging this pull request may close these issues.

3 participants