readthedocs / ext-theme

Read the Docs drop in replacement site templates
2 stars 2 forks source link

Port CAS related changes #307

Closed stsewd closed 7 months ago

stsewd commented 8 months ago

Normal login

Screenshot 2024-03-19 at 13-15-21 Log in - Read the Docs

Login restricted to one provider

Screenshot 2024-03-19 at 13-15-14 Log in - Read the Docs

Login when trying to access a documentation page

Screenshot 2024-03-19 at 12-52-21 Log in - Read the Docs

Screenshot 2024-03-19 at 13-09-19 Log in - Read the Docs Screenshot 2024-03-19 at 13-09-15 Log in - Read the Docs

Closes https://github.com/readthedocs/ext-theme/issues/276

agjohnson commented 8 months ago

This looks great! I'll jump into the code next, but a couple things just looking at the screenshots:

I have a few fixes we can lump together to address layout issues: making room for a third tab, fixing floating issues on the button and checkbox, and maybe making the tabs a bit more obvious.

humitos commented 7 months ago

Showing the email login tab as disabled rather than hide it completely gives the user a bit more information

👍🏼

humitos commented 7 months ago

This is looking good 💯 . I haven't check the code, but I have some feedback from the screenshots:

stsewd commented 7 months ago

When accessing a documentation, can we check if the organization has SSO enabled and only show that provider? That way, there will be one and only one way to login.

I think that's already done

https://github.com/readthedocs/readthedocs-corporate/blob/7dc57dea6cbf9b9a258ff9165650e613b4522c1d/readthedocsinc/proxito/views.py#L57-L64

When accessing the documentation, can we show the "Do you have a password?" option only if the organization has at least one non-expired password?

That feels leaky from our part

stsewd commented 7 months ago

@agjohnson no idea how to fix the width

Screenshot 2024-03-21 at 11-03-23 Log in - Read the Docs Screenshot 2024-03-21 at 11-03-00 Log in - Read the Docs

Also, I put a title as a help text when the email tab is disabled, but it doesn't show as the tab is disabled :/ if we are going to disable that tab, I think we should put some context as why.

agjohnson commented 7 months ago

no idea how to fix the width

The menu bar is a .ui.two.item.menu. If you conditionally change that to .ui.three.item.menu it should keep the width correct. I can help adjust the form visually afterwards, as this will compact all the buttons.

if we are going to disable that tab, I think we should put some context as why.

Ideally we would, yeah. Let's put this on the follow up list, I can help polish these things afterwards. I feel at least with the option disabled that's some amount of visual information. Maybe we end up disabling the tab form instead though.

Can we make the "Forgot your password?" text less prominent?

Let's put this on the follow up list with other visual tweaks too.

stsewd commented 7 months ago

Screenshot 2024-03-21 at 12-31-41 Log in - Read the Docs Screenshot 2024-03-21 at 12-31-16 Log in - Read the Docs Screenshot 2024-03-21 at 12-31-03 Log in - Read the Docs

agjohnson commented 7 months ago

Looking great, I'll take a look at the code and have opened up an issue to address all of the visual fiddling next: