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

Add: tailwindcss view support #114

Merged
merged 65 commits into from
Dec 17, 2022
Merged

Conversation

benkoshy
Copy link
Contributor

@benkoshy benkoshy commented Jun 7, 2022

HI @janko

Please find attached a work in progress PR for Tailwind view support. Some screenshots:

image

image

Notes

  • the _login_form_footer is empty.

  • I have only created it for the main pages: e.g. login, reset passwords etc. The following views are still yet to be done:

    /tailwind/add_recovery_codes.html.erb
    /tailwind/recovery_auth.html.erb
    /tailwind/recovery_codes.html.erb
    /tailwind/two_factor_auth.html.erb
    /tailwind/two_factor_disable.html.erb
    /tailwind/two_factor_manage.html.erb
    /tailwind/unlock_account.html.erb
    /tailwind/unlock_account_request.html.erb
    
  • To save you time, I have already created a tailwind demo app which you can spin up in an instant.

  • I have retained all the original templates in the rodauth directory. You may decide to put the default files, rather, in a bootstrap directory.

  • The views should be responsive, mobile first, and have dark mode enabled as well. Some of the dark mode views don't work for the full page. I will come back and fix this.

Please do not hesitate to send things back to me to fix etc: tt might be a little more work to delegate, true, but the investment might be worth it.

@janko
Copy link
Owner

janko commented Jun 8, 2022

Looks pretty good 🙂. I noticed that Tailwind templates have extra text, would it be possible for them to have the same page content as the Bootstrap ones? This way they will be more similar to Bootstrap ones, and they will be fully internationalized when switching locales (e.g. when using rodauth-i18n).

@benkoshy
Copy link
Contributor Author

benkoshy commented Jun 8, 2022

Questions

Looks pretty good slightly_smiling_face. I noticed that Tailwind templates have extra text, would it be possible for them to have the same page content as the Bootstrap ones? This way they will be more similar to Bootstrap ones, and they will be fully internationalized when switching locales (e.g. when using rodauth-i18n).

  • I don't understand what is meant by standardizing the text: could you pls elaborate?

Items to Confirm

Please also confirm the following items:

  1. Default bootstrap views to be placed in a new bootstrap folder: Yes/No
  2. A separate rodauth layout to contain all rodauth views: Yes/No

Why? Because if you use the standard application.html.erb layout then people might be using all sorts of classes there which would mess up the rodauth styling. Allusion to this was made in #109. This might result in a heavy support/maintenance burden:

<main class="container mx-auto mt-28 px-5 flex">  <---- Note the classes here            
</main>

Others might do this:

<main> <---- No classes noted here      
</main> 

I can change the generators to reflect this.

  1. Whether the tests are adequate? Yes/No

ETA

  • I hope to finish add a few forms by this Friday. So perhaps completion in one or two weeks.

@janko
Copy link
Owner

janko commented Jun 8, 2022

I don't understand what is meant by standardizing the text: could you pls elaborate?

For example, the login form has "Sign In" heading, and "Don't have an account? Sign up here", neither of which are present in the original Bootstrap templates. So, it's extra text/content.

If I use rodauth-i18n and choose a locale with built-in translations (e.g. Portugese), these chunks of custom text will stay in English. This provides a different experience compared to default Bootstrap templates, which will be fully translated.

I will answer the rest later today 🙂

@janko
Copy link
Owner

janko commented Jun 8, 2022

Default bootstrap views to be placed in a new bootstrap folder: Yes/No

I'm leaning on the side of keeping the default bootstrap views where they are, to signal that these match Rodauth's built-in templates, making them official in a sense.

A separate rodauth layout to contain all rodauth views: Yes/No

If we had a special layout, we would also need to make it opt-outable, and I would prefer not to go down that route. I would like to keep the simplicity the Bootstrap views have. Having <main> be a horizontal flexbox doesn't seem like a good default to me, I don't expect many people to have that.

Whether the tests are adequate? Yes/No

They look good to me 👍🏻

@benkoshy
Copy link
Contributor Author

@janko Sir,

This way they will be more similar to Bootstrap ones, and they will be fully internationalized when switching locales (e.g. when using rodauth-i18n).

I felt that it was important to allow for additional sign-in options (i.e. sign in via Google / Twitter) within the original tailwind template - this may conflict with the design intent of the library....but if I can make it work with rodauth-i18n would you object? In other words, the forms will have some slight differences, but will be fully internationalized. pls confirm, and I can do accordingly as per your directives.

rgds
Ben

@janko
Copy link
Owner

janko commented Jun 14, 2022

I felt that it was important to allow for additional sign-in options (i.e. sign in via Google / Twitter) within the original tailwind template - this may conflict with the design intent of the library

I don't think it makes sense to do that if Rodauth itself doesn't provide this functionality. I get that it might have been part of a Tailwind UI template, but it's not suitable for rodauth-rails, the default templates should only contain functional elements.

but if I can make it work with rodauth-i18n would you object? In other words, the forms will have some slight differences, but will be fully internationalized. pls confirm, and I can do accordingly as per your directives.

Having additional translations just for Tailwind templates would be an unnecessary maintenance burden. It couldn't be part of the rodauth-i18n project, because that gem can be used without Rails, so Tailwind templates would likely be missing translations for many languages that get added to rodauth-i18n.

Correct me if I'm wrong, but wasn't the goal of Tailwind templates to have a minimal thing that looks good when you're using Tailwind, just to give you a starting point? In that sense, having templates that look exactly the same as Bootstrap but are styled with Tailwind would technically fulfill that goal. I'm not saying this is necessarily what I want, I just think we should strive for simplicity here.

@benkoshy
Copy link
Contributor Author

benkoshy commented Jun 17, 2022

@janko I"m still here chipping away at this. Please bear with me. Because i'm new to rodauth, small things which are easy, are taking x10 times the amount of time.

@benkoshy benkoshy marked this pull request as draft June 24, 2022 07:14
@kyle-rader
Copy link

Oh my gosh, I have also been adding Tailwind styles to the rodauth views in my own project this last week. These look great so far, I'd love to adopt them!

@benkoshy
Copy link
Contributor Author

@kyle-rader Please be patient with me - I'm still trying to grok the rodauth and rodauth-rails library so i get stuck really easily :'( ---> they're small things, but big enough to hold me up..........only a few more forms are left to be done, which i will steadily chip away at.

@benkoshy
Copy link
Contributor Author

More snips. Compatible in dark mode in the latest absolute latest version of rodauth. working through the last few multifactor forms

image

image

@benkoshy benkoshy force-pushed the add-tailwind-css-forms branch 2 times, most recently from 55b3bdf to e478fc1 Compare September 9, 2022 02:31
@benkoshy benkoshy force-pushed the add-tailwind-css-forms branch 3 times, most recently from 2f83cc0 to d1ef29f Compare October 4, 2022 00:36
fix ui: move position of heading
Previous versions of rails i.e. Rails 5 are still supported. However,
class_names are not compatible with this version of rails.
@benkoshy benkoshy marked this pull request as ready for review October 4, 2022 01:12
@benkoshy
Copy link
Contributor Author

benkoshy commented Oct 4, 2022

@janko Feel free to try out it out on the demo app here: https://github.com/benkoshy/rodauth-rails-tailwind-demo - instructions contained therein.

Also I have hard-coded to a dark-mode color. This may not be the best outcome. Perhaps it is better to specify dark mode for the elements within the form, rather than the dark mode itself. This will be patent on testing the demo app above.

Feel free to push back onto me anything that needs reworking.

@janko
Copy link
Owner

janko commented Oct 4, 2022

When I look at the login page, I see numerous issues:

  1. input fields are red by default (which is bad UX, we should probably rely just on server-side validation errors)
  2. when I focus a valid field, the blue focus outline mixes with the green border
  3. labels are too light, the contrast doesn't even meet accessibility standards (why not black labels?)
  4. that box shadow looks out of place (maybe it shouldn't be there?)
  5. the inline "Forgot Password?" is not vertically aligned with "Login" label (do we really need it, given that we already have one at the bottom?)
  6. input fields seem too narrow (a longer email/password won't fit)

Screenshot 2022-10-04 at 10 46 56

In the dark mode, I think it would be better if forms just inherited the background color of the container, rather than having their own. That's how Bootstrap templates behave, and it seems like a more sensible default.

When I look at the create account form, the color of labels and spacing between fields is totally different from the login form. The form layouts should all be consistent.

Screenshot 2022-10-04 at 11 01 34

Also, when I cause validation errors in the create account form, for some reason the input fields expand to double the length 🤷🏻‍♂️. Also, the validation error message color is different from the border of the invalid field, and I think it would look better if it was the same.

Screenshot 2022-10-04 at 11 03 21

In the TOTP setup form, the input fields are way too narrow, the "Authentication Code" label even spans multiple lines, not to mention that the button text is cut off.

Screenshot 2022-10-04 at 11 16 58

I see many more things:

  • in the reset password request form, the login field has a "login" placeholder (which is inconsistent with other forms which don't have any placeholders)
  • in the verify account resend form, the login field doesn't have a label (which is inconsistent with other forms)
  • the change password/login forms display at the bottom of the page for some reason

Also, there seem to be some merge conflicts with the test file for the views generator.

@benkoshy
Copy link
Contributor Author

benkoshy commented Oct 4, 2022

I appreciate the feedback.

I will incorporate the above suggestions.

I propose then to move forward incrementally:

  1. let me get one form absolutely correct: e.g. the login page.
  2. And then apply the same model to the rest, moving incrementally.

@benkoshy benkoshy marked this pull request as draft October 4, 2022 22:59
@benkoshy
Copy link
Contributor Author

benkoshy commented Nov 3, 2022

@janko I beg your patience. I have had some constraints on my end. I wish to see this through to completion. Fingers crossed by next week.

@benkoshy
Copy link
Contributor Author

benkoshy commented Nov 11, 2022

Hi @janko - I have updated just one form - the login form in light of your comments - if you find this adequate, I will update the same across all the forms. If you have a spare 5 min - i would not spend any more than that - and could give it a review, I would appreciate that

input fields are red by default (which is bad UX, we should probably rely just on server-side validation errors)
when I focus a valid field, the blue focus outline mixes with the green border
labels are too light, the contrast doesn't even meet accessibility standards (why not black labels?)
that box shadow looks out of place (maybe it shouldn't be there?)
the inline "Forgot Password?" is not vertically aligned with "Login" label (do we really need it, given that we already have one at the bottom?)
input fields seem too narrow (a longer email/password won't fit)

Link: https://github.com/benkoshy/rodauth-rails-tailwind-demo
./bin/dev

Route: http:https://localhost:3000/login

(I will also remove the hard coding of the particular dark-mode color)

Please do not hesitate to send back any problems. thank you for your patience.

Ben

@janko - have i gone off the track?

@janko janko marked this pull request as ready for review December 17, 2022 12:54
@janko janko merged commit 2e1dd1c into janko:main Dec 17, 2022
@janko
Copy link
Owner

janko commented Dec 17, 2022

I ended up merging the current work, and fixed various issues I encountered, added some improvements, and simplified markup where it made sense. Thanks for all your hard work in providing a great base for this feature 🙏🏻

@benkoshy
Copy link
Contributor Author

benkoshy commented Dec 17, 2022

I ended up merging the current work, and fixed various issues I encountered, added some improvements, and simplified markup where it made sense. Thanks for all your hard work in providing a great base for this feature 🙏🏻

That would have taken some time! Your contributions have been immense - I regret i could not have alleviated your workload more. 😔

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

3 participants