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

fix: focus offset added for checkbox and buttons #390

Merged
merged 2 commits into from
Dec 5, 2024

Conversation

abdul99ahad
Copy link
Contributor

@abdul99ahad abdul99ahad commented Nov 26, 2024

Related to camunda/camunda-modeler#4632

Proposed Changes

Fixed the focus outline offset

Checkbox:
image

Button:
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}

Related to: camunda-modeler #4632
@abdul99ahad abdul99ahad force-pushed the focus-indicators-properties-panel branch from b23f58e to bcc1399 Compare December 2, 2024 11:39
@@ -537,8 +544,9 @@ select.bio-properties-panel-input {
}

.bio-properties-panel-input[type="checkbox"]:focus {
outline: 2px solid var(--input-focus-border-color);
outline-offset: 0;
outline: auto;
Copy link
Contributor

@philippfromme philippfromme Dec 2, 2024

Choose a reason for hiding this comment

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

Why auto? It let's the user agent render a custom outline style. Is this what we want?

Copy link
Member

@nikku nikku Dec 2, 2024

Choose a reason for hiding this comment

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

Two directions to take here:

  • It is integrators responsibility (Desktop Modeler, Web Modeler, bpmn.io demo, ...) to decide on outline (everything or subset, i.e. "you can decide on color")
  • We offer reasonable defaults, and folks may override, i.e. to better embed in a 90s style website.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then I'd expect it to be consistent. If we already set the color we should also set the style. As you said, one can always override.

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 agree. We should maintain consistent styling across all child libraries instead of relying entirely on integrators. While we can allow them the flexibility to override styles, our defaults should not depend on their configurations.

Copy link
Contributor Author

@abdul99ahad abdul99ahad Dec 3, 2024

Choose a reason for hiding this comment

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

We've already specified the color in the same class. Using the auto property ensures an default outline is provided, overriding the none outline for inputs.

.bio-properties-panel-input:focus,
.bio-properties-panel-input:focus-within {
  outline: none;

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.

Looks good to me, looking to @philippfromme for final check.

@philippfromme
Copy link
Contributor

Are we okay with this outline still being black?

image

@nikku
Copy link
Member

nikku commented Dec 4, 2024

Yes, because outline color is setup by the application.

@nikku nikku merged commit 27434f3 into main Dec 5, 2024
12 checks passed
@nikku nikku deleted the focus-indicators-properties-panel branch December 5, 2024 15:04
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Dec 5, 2024
@nikku
Copy link
Member

nikku commented Dec 5, 2024

@abdul99ahad ready to be released and integrated ✔️

@abdul99ahad abdul99ahad mentioned this pull request Dec 9, 2024
4 tasks
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