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

Login page rework #623

Merged
merged 31 commits into from
Jan 23, 2024
Merged

Login page rework #623

merged 31 commits into from
Jan 23, 2024

Conversation

ElvisAns
Copy link
Member

@ElvisAns ElvisAns commented Sep 20, 2022

Pullrequest

Issues

  • The The login page UI

Checklist

  • None

Screenshots

image

image

How2Test

  • Just try to login and see the new design

Todo

  • Propose any other change to this proposal

@ElvisAns ElvisAns mentioned this pull request Sep 20, 2022
@jasonmunro
Copy link
Member

I think this looks great, but it needs a couple fixes and I would prefer to make this optional based on a site configuration value. I will definitely use it for my personal site but I would prefer to keep the original plain login as the default and sites can opt-in to use the fancy version.

I did not test the recaptcha module set yet to see if it lines up ok but I can do that after we sort out the issues. Specifically the way the 2 image files are being included is incorrect. In order to work with different site setups includes should be prefixed with the WEB_ROOT constant, and assets should be placed in a module set directory. So please move both images to

 modules/core/assets/

and change the URLS to:

src="'.WEB_ROOT. 'modules/core/asset/images/logo.svg"
url('.WEB_ROOT.'modules/core/asset/images/cloud.png);'

Note you have to run the config_gen.php script for the assets to be properly copied over to the production version of the site. Thanks!

@ElvisAns
Copy link
Member Author

Now everything follow your guidance. Thank you @jasonmunro .
Next is to know how are we going to make it optional, maybe we need to add switch button somewhere and a file or database entry to save the user setting so he may switch between the login screen accordingly...

@jasonmunro
Copy link
Member

Now everything follow your guidance. Thank you @jasonmunro . Next is to know how are we going to make it optional, maybe we need to add switch button somewhere and a file or database entry to save the user setting so he may switch between the login screen accordingly...

This does not need to be a user setting but a site level setting. Lets add it to the hm3.ini file, then use that to determine which login page to show.

@ElvisAns
Copy link
Member Author

I got the point.
I am going to work on it soon and give you update

@ElvisAns
Copy link
Member Author

ElvisAns commented Nov 1, 2022

@jasonmunro the login page UI is know configurable.

Thanks

@ElvisAns
Copy link
Member Author

ElvisAns commented Jul 4, 2023

@kroky I have a question, I just want to know more about cypht.
I know module handler classes are like controller and the order in which their process() methods are called depends on where they have been attached from the module setup.php file, now i need to know, output modules are called just in the naming in which they are declared?

  • Eg we have handler module called Hm_Handler_login (technically login) then it means that all classes that have login in their name will be automatically called when the login handler is executed? like Hm_Output_login_end will automatically be called to close tags that were opened in Hm_Output_login_start and continued in Hm_Output_login ?

@kroky
Copy link
Member

kroky commented Jul 5, 2023

@ElvisAns, no, I don't think there is such a logic. The output modules are defined much like handler modules in setup.php and are executed in the order defined.

@marclaporte
Copy link
Member

For the record: It's fine to start this as "optional based on a site configuration value"

But each option is a potentially untested code path. After it's well tested, and we improve design/themes overall , we may remove the option, and everyone will get the fancy login. This will be in a major release (ex.: from 1.6.x to 1.7.x) so users expect some changes.

@josaphatim
Copy link
Member

@ElvisAns can you rebase locally ?

Copy link
Member

Choose a reason for hiding this comment

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

What is the source and license of this image?

Copy link
Member Author

Choose a reason for hiding this comment

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

@marclaporte maybe we need to replace it ... TBH i dont really remember where i have picked the image also this PR started more than 16months ago

Copy link
Member Author

Choose a reason for hiding this comment

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

I have replaced the image with a free one https://www.pexels.com/photo/heavy-clouds-1828305/

@josaphatim josaphatim merged commit 6b0d2dd into cypht-org:master Jan 23, 2024
Yannick243 pushed a commit to Yannick243/cypht that referenced this pull request Feb 1, 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.

None yet

5 participants