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

Standalone login improvements (message/theme/events) #31149

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

artfulrobot
Copy link
Contributor

Various standalone login related items:

  1. improves the standalone login and TOTP screens by using set status message + standard display of messages ("you have just been logged out" etc.)
  2. Adds a new event class and fires it a bunch of times during the login process. There are no listeners to these events, but extensions could listen to them to implement things like flood control/monitoring etc.
  3. Some code comment updates/clarifications.
  4. TOTP: minor improvements: clear the field if a TOTP code was invalid; focus the field on page load.
  5. Minor CSS change to login screens: position the form box up a bit; it was technically vertically centred but this did not visually appear centred due to the logo; i.e. it looked not-centred/weirdlylow on the page and now it's a bit nearer the top. Meh, but nicer.

Copy link

civibot bot commented Sep 20, 2024

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Sep 20, 2024
@artfulrobot
Copy link
Contributor Author

@ufundo if you wouldn't mind doing the r-run testing for this please.

@vingle I know you like screenshots...

Before: The bit you type in "feels" at the bottom of the page, which feels wrong to me.

image

After: content is nearer where you expect it.

image

@ufundo
Copy link
Contributor

ufundo commented Sep 20, 2024

Good

r-ran on the test site and all works as expected for me.

Code looks fine to me.

Blocking

Couple of test fails were the test is looking for specific wording so need updating with the notification copy changes

(The upgrade test is new and the fail is not this PRs fault)

Non-blocking

I think I liked the login box lower down for some reason but 🤷‍♂️

Also noticed on stock Greenwich the inputs don't align so nice

image

This wasn't a problem before because they were underneath

image

If the html makes sense and is themeable then I can live with it. @vingle ?

@artfulrobot
Copy link
Contributor Author

artfulrobot commented Sep 20, 2024

re wide inputs - yeah, I know, I liked them big too and I was trying to make them "huge" and testing with riverlea but couldn't get them to behave. I thought it was likely a RL bug, but maybe not. EDIT: I've added more CSS to override greenwich and make those block layout again, so they now look like your 2nd screenshot again.

will check tests. thanks @ufundo

@vingle
Copy link
Contributor

vingle commented Sep 20, 2024

.huge is 25em across Riverlea, which is a direct copy from core/greenwich civicrm.css's .huge. But that overwrites the 100% width set on input here, and so it wraps onto the same line creating the effect above.

@artfulrobot
Copy link
Contributor Author

artfulrobot commented Sep 20, 2024

@vingle thanks, I've removed huge. (I had thought it was a new RL thing, and on my thames theme I'd set it to 100%... didn't realise it was an old rule.)

@vingle
Copy link
Contributor

vingle commented Sep 21, 2024

@artfulrobot - I hadn't initially included it, but then noticed some inputs on the add new contact page were larger than others.

Probably more elegant ways to do that than 25em but seemed easier to replicate. Maybe it better in _fixes.cms as it's more legacy support, than how you'd do that now.

This gives other extensions opportunity to implement various extra
login guards, e.g. excessive wrong passwords/mfa attempts.

standalone: Login/TOTP improve notifications using status messages

standalone: minor css improvement for login

standalone: shrink QR code a bit, it was mahusive

standalone: remove API_Exception → CRM_Core_Exception

fix style

Update tests for changed wording in standalone

standalone: remove huge - we want it *bigger*!

style fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master run-standalone Civibot should setup demos+tests for Standalone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants