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: set document title #4377

Merged
merged 1 commit into from
Jun 18, 2024
Merged

fix: set document title #4377

merged 1 commit into from
Jun 18, 2024

Conversation

barmac
Copy link
Collaborator

@barmac barmac commented Jun 18, 2024

image

Closes #4376

Proposed Changes

This PR replaces the main process code used for the window title with an HTML <title> tag. The content is hard-coded as "Camunda Modeler" in contrast to the old solution where we added "(dev)" for dev mode, but this piece of information is already represented in the bottom-right "what's new" button. The main advantage of the current solution is that the a11y tools won't report missing title anymore.

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 Jun 18, 2024
@barmac barmac changed the base branch from develop to main June 18, 2024 09:08
Copy link

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

Consider targeting main instead.

@barmac barmac requested review from a team, philippfromme and marstamm and removed request for a team June 18, 2024 09:08
@@ -419,7 +419,6 @@ app.createEditorWindow = function() {
const windowOptions = {
resizable: true,
show: false,
title: 'Camunda Modeler' + getTitleSuffix(app.metadata.version),
Copy link
Member

Choose a reason for hiding this comment

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

I personally found this useful, why not keep it?

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a duplicate of
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we keep the code above, we set the title in two places:

  1. main process
  2. HTML

But on MacOS the HTML part takes precedence.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Long term, we could consider adding context to the title, e.g. the current tab. But it's outside of the scope of the linked issue.

Copy link
Member

Choose a reason for hiding this comment

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

OK, let's decide here and now that we ditch dev tag, and if we ever add it (again), then we do it inside the app.

@barmac barmac merged commit 5592f1e into main Jun 18, 2024
15 checks passed
@barmac barmac deleted the 4376-document-title branch June 18, 2024 10:04
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Jun 18, 2024
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.

Set window title via HTML
2 participants