fix(menubar): show selection status correctly

[Hephi2]

Buttons in the menubar like bold, italic, underline and strikethrough are show now if they are selected or not right after the selection.

📝 Summary

• Resolves: Edit icons not updated properly #3907

Before the fix the is-active status of the icon buttons for bold, italic, underline and strikethrough did not change after selecting. Now the status is updated after the click to show if the modifiers are active even before starting to write with those.

[cypress]

1 flaky tests on run #9922 ↗︎

0

142

1

0

1

Details:

fix(menubar): show selection status correctly
Project: Text	Commit: b68b18c712
Status: Passed	Duration: 07:14 💡
Started: May 23, 2023 11:35 AM	Ended: May 23, 2023 11:42 AM

  cypress/e2e/sync.spec.js • 1 flaky test

View Output Video

Test		Artifacts
Sync > recovers from a lost connection		Output Screenshots

_{This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.}

[juliushaertl]

Nice, change looks good. I think this is also a good case to cover with a simple cypress test that asserts that the button has the active state class.

@max-nextcloud Maybe you can look into that together with @Hephi2 next week and guide him a bit on the first steps with cypress :)

[Hephi2]

I noticed that I also have to fix this bug for for when using the shortcuts.

[max-nextcloud]

All in all looks great. Thanks a lot for looking into this. I added a few comments to the eslint complaints. Hope that makes it easier to understand and fix them.

When in doubt please ask in the talk channel.

[max-nextcloud]

There are a few formatting issues reported by eslint like this one.
No trailing whitespace should follow the {.

In order to see these more easily it might be useful to install an eslint extension in you vscode. That should highlight such issues automatically.

[max-nextcloud]

The extra ; at the beginning of the line was here because the line started with an array:

cy.getContent().type('{selectall}')
;[

Without a ; Javascript would interpret the [ as an index into an array (as in cy.getContent().type('{selectall}')[1])

Now that you are using a variable (much better) there's no more need for the semicolon.

[max-nextcloud]

Cypress eslint extension is complaining about these lines because you are chaining off commands.
What does it mean by that:

cy.getContent().type('Format me{selectall}').find(...)

.type() is a command. (So is .click()).

The problem is rooted in how cypress works. For queries like find() it will look for the element. If it does not exist yet it will retry until the element exists or it times out.
If the find() command is chained from the type() command there's no safe way for cypress to retry without calling the type() command multiple times.

So whenever you have a command that actually changes something you need to end the chain:

Suggested change

	cy.getContent().type('Format me{selectall}')
	.find(`${tag}`)
	.should('contain', 'Format me').type('{del}')
	cy.getMenuEntry(button).click().should('have.class', 'is-active').click().should('not.have.class', 'is-active')
	cy.getContent().type('Format me{selectall}')
	cy.getContent().find(`${tag}`)
	.should('contain', 'Format me').type('{del}')
	cy.getMenuEntry(button).click()
	cy.getMenuEntry(button).should('have.class', 'is-active').click()
	cy.getMenuEntry(button).should('not.have.class', 'is-active')

This is only checked since a short time - so you will still find old documentation and even code that does not follow this rule.

[Hephi2]

Thanks for the detailed feedback!

[max-nextcloud]

Looks really good. Thanks for diving into this!

🧪 Bonus points for tests included 💯 😉

[max-nextcloud]

Failing cypress test is known to fail from time to time.

[Hephi2]

/compile

[blizzz]

if that's a bugfix that has to go to 27, it needs backporting for we are behind branch-off-