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

Search improvements #913

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

Search improvements #913

wants to merge 14 commits into from

Conversation

philippfromme
Copy link
Contributor

@philippfromme philippfromme commented Jun 3, 2024

Improves search by

  • treating search as truly intermediate (root element, selection and viewbox reset on escape without click or enter)
  • aligning styles with other popups
  • simplifying selection visuals when "pre-selecting" elements before selecting (click or enter)
    • simply use existing outline
    • don't show context pad during pre-select

brave_MvTovcBG1j

Related to bpmn-io/bpmn-js#2187

@@ -525,7 +486,7 @@ function createHtmlText(tokens) {

tokens.forEach(function(t) {
if (t.matched) {
htmlText += '<strong class="' + SearchPad.RESULT_HIGHLIGHT_CLASS + '">' + escapeHTML(t.matched) + '</strong>';
htmlText += '<b>' + escapeHTML(t.matched) + '</b>';
Copy link
Member

Choose a reason for hiding this comment

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

Why do we remove the ability to customize here?

Copy link
Member

Choose a reason for hiding this comment

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

Also, let's keep using semantic html. <strong> for important stuff, <em> for emphasis.

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 can revert the CSS class removal.

For <b> vs <strong> I'd disagree based on what I read.

The element is for content that is of greater importance, while the element is used to draw attention to text without indicating that it's more important.

I'd say we only want the styling, not the importance.

Each element is meant to be used in certain types of scenarios, and if you want to bold text for decoration, you should instead actually use the CSS font-weight property.

So you could even argue we don't want to use tags at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In terms of a11y, <strong> matters to screen readers while <b> doesn't which is what I'd expect in that case. I wouldn't expect the screen reader to focus on specific parts of the results.

Copy link
Member

Choose a reason for hiding this comment

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

👍 for "just for decoration.

Copy link
Member

@barmac barmac Jun 5, 2024

Choose a reason for hiding this comment

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

I like how the tag meaning was changed from "<b>oldface" to "<b>ring attention". I'm ok with leaving it as is.

@philippfromme philippfromme marked this pull request as ready for review June 5, 2024 14:05
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jun 5, 2024
@nikku
Copy link
Member

nikku commented Jun 24, 2024

Heads-up: I reviewed this but somehow the remarks were lost. Going to repeat, once back from FTO.

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

Successfully merging this pull request may close these issues.

None yet

3 participants