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

ENH Update styles to match new RealME branding #113

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Jun 23, 2023

Notes

  • There's a lot of css here that doesn't seem to be used - but it's possible that it's just used somewhere that I wasn't able to identify.
  • "MiniLoginForm" is unusable so I've left its styling alone. Given that nobody has complained that it's broken it's obviously not being used, so it's not worth figuring out its styling as part of this issue IMO
  • The "What's realme" popup javascript doesn't work, but I temporarily made that popup visible by manipulating the DOM directly and its styling isn't against the guidelines from what I could tell so I left it alone.
  • To get the form to display, I followed the configuration documentation, using "mts" since that's recommended for local dev and doesn't require signing up for anything or getting RealME to assign us any certificates etc - but it was still a bit of a process. I'm happy to help whoever reviews this if you want to try set it up that way for testing, but it may be easier to just add the yml config from that documentation and then manually slap a SilverStripe\RealMe\Authenticator\LoginForm somewhere. It won't let you log in that way obviously but a) I couldn't log in either even doing the "mts" thing and b) this is just about styling, so it doesn't matter if you can log in or not.

Changes made

  • Remove "theme" styling
    • There isn't enough in the brand guidelines to build a dark mode that conforms with their guidelines
    • I left the theme swapping mechanism in tact, so that people can implement their own theme styling if they want to - but by default it doesn't do anything
  • Updated styling for main form
    • I mainly used the login form on the realme website itself as a guide, but I did grab the colour hex codes and icons from their guidelines
  • Migrated CSS to SCSS
    • This is to reduce merge-up pains, since this was done in CMS 5 already. See note on the issue about merge-ups.

Example of change

OLD (default theme)

integration_type set to "login"

old styling of the realme login screen

integration_type set to "assert"

old styling of the realme assert screen

NEW

integration_type set to "login"

new styling of the realme login screen

integration_type set to "assert"

new styling of the realme assert screen

Issue

@GuySartorelli GuySartorelli force-pushed the pulls/4/branding branch 2 times, most recently from 3d2998b to 277b0f3 Compare June 23, 2023 00:37
@GuySartorelli
Copy link
Member Author

GuySartorelli commented Jun 23, 2023

NOTE: The js test fails because there's no client/dist/ directory - but I can't move the css to the client/dist/ directory without potentially breaking a scenario where someone is using Requirements::block('silverstripe/realme:client/css/realme.css');

@import 'links';
@import 'popup';
@import 'icons';
// @import 'themes';
Copy link
Member

Choose a reason for hiding this comment

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

Remove

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

FILES_PATH: '../',
ROOT: Path.resolve(),
SRC: Path.resolve('client/src'),
DIST: Path.resolve('client/css'),
Copy link
Member

Choose a reason for hiding this comment

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

Can you make it so that uses client/dist (or whatever the standard path is) so that the JS CI jump passes? If you're still concerned about backwards compatibility then add in a copy step that copies to the legacy folder and add a comment in here that it's for BC

Copy link
Member Author

Choose a reason for hiding this comment

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

Done by building the file twice, since I can't use the webpack copy plugin to copy files after they're built and a cp in the package.json build script wouldn't be used by CI.

<% if ShowNewWindowIcon %><span class="realme_icon_new_window"></span><% end_if %> <span class="realme_icon_padlock"></span>
<span class="realme_button_login-icon"></span>
<span class="realme_button_login-text">
$Label <% if ShowNewWindowIcon %><span class="realme_icon_new_window"></span><% end_if %></span>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$Label <% if ShowNewWindowIcon %><span class="realme_icon_new_window"></span><% end_if %></span>
$Label <% if $ShowNewWindowIcon %><span class="realme_icon_new_window"></span><% end_if %></span>

Copy link
Member Author

@GuySartorelli GuySartorelli Jun 26, 2023

Choose a reason for hiding this comment

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

I didn't want to touch the stuff that was already there that didn't need changing. But this is effectively a no-change so I'll do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Have viewed demo of these components on Guys computer

@emteknetnz emteknetnz merged commit 5dfdf4f into silverstripe:4 Jun 26, 2023
@emteknetnz emteknetnz deleted the pulls/4/branding branch June 26, 2023 04:27
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.

2 participants