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

XWIKI-22191: Account disabling and password changing action links should be buttons #3148

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented May 30, 2024

Jira URL

https://jira.xwiki.org/browse/XWIKI-22191

Changes

Description

  • Updated the account disabling buttons
  • Updated the Change password button to be a button instead of an anchor styled like a button
  • (Extra - not required by the ticket, but close to its subject) Improved the consistency of the inline-link-only selector to also match .button links, which can happen, such as the one for the creation of a new wiki.

Clarifications

Screenshots & Video

None, abrely any visual change. The link underlining doesn't happen when hovered on both of those buttons anymore.

Executed Tests

Manual tests for both of the buttons. They both still had the same effect after the DOM changes.

Expected merging strategy

…uld be buttons

* Updated the `Change password` button to be a button instead of an anchor styled like a button
@@ -183,7 +183,8 @@ body {

/* Remove the underlining from links with the btn class.
Those are already styled differently from their neighboring text. */
& a.btn {
& a.btn,
& a.button {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this change, you didn't introduced a button class here, but you replaced a link to a button, so the selector should be just button no?

Copy link
Contributor Author

@Sereza7 Sereza7 Nov 27, 2024

Choose a reason for hiding this comment

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

image

When checking out the displays of "button like links", I stumbled upon this one. It gets underlined on hover but there's no need for it to be underlined. See the third item of the PR description in the opening message.

@@ -340,8 +340,8 @@
<div class="passwordManagement">
<h2>$escapetool.xml($services.localization.render('platform.core.profile.section.security'))</h2>
<span class="buttonwrapper">
<a id="changePassword" href="$doc.getURL('view', 'xpage=passwd')">$escapetool.xml(
$services.localization.render('platform.core.profile.changePassword'))</a>
<button id="changePassword" onclick="location='$doc.getURL('view', 'xpage=passwd')'">$escapetool.xml(
Copy link
Member

Choose a reason for hiding this comment

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

Hmmmm it means it won't work anymore when javascript is disabled. Not a big fan of this onclick attribute to be honest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted those changes in d043470 👍

Copy link
Member

@surli surli left a comment

Choose a reason for hiding this comment

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

I understand the need for disable/enable button, which really aren't links. But for the changePassword I don't understand why you changed it: it's really a link here so IMO it should stay like it was.

@Sereza7 Sereza7 marked this pull request as draft October 24, 2024 08:39
@Sereza7
Copy link
Contributor Author

Sereza7 commented Nov 27, 2024

I understand the need for disable/enable button, which really aren't links. But for the changePassword I don't understand why you changed it: it's really a link here so IMO it should stay like it was.

It's fair. There was no feedback about this on the forum topic. I'll update the changes to keep this an anchor but avoid underlining on hover.

…uld be buttons

* Set the changePassword element back to an anchor
* Updated the underlining rules to count this one as a `link styled like a button` category
@Sereza7
Copy link
Contributor Author

Sereza7 commented Nov 27, 2024

Resolved merge conflict and addressed comments.
This PR is ready for more reviewing/merging.

@Sereza7 Sereza7 marked this pull request as ready for review November 27, 2024 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants