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

Adds more control over login module form style #3270

Closed
wants to merge 3 commits into from

Conversation

thabaum
Copy link
Contributor

@thabaum thabaum commented Nov 10, 2019

In reference to these issues:
#3269
#2471

Summary

Creates additional css class's to allow more independent styles to be applied to login module page.

@thabaum
Copy link
Contributor Author

thabaum commented Nov 10, 2019

I will see about doing this for the Registration page as well if everyone is ok with how it was done. I studied a few things from the previous CSS classes and figured this approach does not break anything existing and people can then add CSS to the Custom CSS or Skin.css. And if we want to get a more grip on the Default.css... this would potentially help however might be a breaking change so before playing with the CSS in default I think just adding this so it can get played with is great start for 9.

I will also provide pull requests for dnndocs.com once any of my PR's get put into an RC if needed.

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

Please do not remove dnnLabel and dnnFormLabel as it may unnecessarily break existing themes. Also can we just add the minimal required classes on each parent group that have distinct elements already, we can use css specificity instead of adding a class to each nested node. For instance instead of .dnnLoginActionLogin, one could simply do .dnnLoginFormItemLogin>.dnnFormLabel thus not requiring adding so many classes. Each child already have unique classes, we could just add one class instead of 4.

@thabaum
Copy link
Contributor Author

thabaum commented Nov 10, 2019

Please do not remove dnnLabel and dnnFormLabel as it may unnecessarily break existing themes.

I intended to add classes only not remove any so this was some sort of error on my part moving down the code in GitHub to edit. Only line I didn't feel i needed to add to was dnnLoginActions as it was unique enough. The others I added a second class copying and pasting but I will have a look at the changes you requested.

I inserted the missing original classes as originally intended. My PR's going forward are to not break anything by design. Nothing should break this PR should only add a second class that is more specific to each object to allow more control over style.

This gives ability to skin the original groups and now specific these objects in the login skin that are presented in this PR if accepted.

Thank you for looking at this and discovering that was missing. It is always something it feels like but it was a late night putting that out while I was thinking about it.

Additional thoughts here:

If the naming convention looks ok someone that knows the login form already will just need to put Login and the name of the object around the current type to modify something specific.

www.dnndocs.com can help explain these exist as well. I need to get busy there for these PR's that are accepted. We could use someone that just helps maintain these updates that are accepted that are worth documenting. I will try to learn how to do that as well for my own PR's that introduce anything new if someone else does not do it first.

It would be nice if some schools such as IT Colleges and Universities could get more involved with the DNN project and help it evolve further with real class projects. Learning how to work with an open source .NET community project trying enhance, maintain or to help move the platform to .NET CORE with some class projects submitting PR's on github. Is that something the community can promote in a class and be acceptable if it works as a solutoin? These kind of ideas to promote ways for other groups that are looking to teach and learn .NET along with helping an open source non profit community.

Nice catch needing to re-add these missing original classes as they had been removed.  This is what I meant to be in the first request, was late night... trying to squeeze in what I can thanks for the heads up once again!
@thabaum thabaum requested a review from valadas November 10, 2019 19:35
@thabaum
Copy link
Contributor Author

thabaum commented Nov 10, 2019

Added CSS Classes to document (I hope I got them all so I can help with the dnndocs as needed):

dnnLoginForm dnnLoginFormItemUsername dnnLoginLabelUsername dnnLoginFormLabelUsername dnnLoginFormItemPassword dnnLoginFormLabelPassword dnnLoginFormItemCaptcha dnnLoginFormLabelCaptcha dnnLoginFormItemMessageCaptcha dnnLoginFormMessageCaptcha dnnLoginFormItemLogin dnnLoginFormLabelLogin dnnLoginPrimaryAction dnnLoginActionLogin dnnLoginSecondaryAction dnnLoginActionCancel dnnLoginFormItemRemember dnnLoginFormLabelRemember dnnLoginFormItemActions dnnLoginFormLabelHelp dnnLoginActionsList dnnLoginActionRegister dnnLoginActionRecover

Original Existing CSS Classes:

dnnLoginRememberMe dnnSecondaryAction dnnActions dnnLoginActions dnnFormLabel dnnFormItem dnnSecondaryAction dnnPrimaryAction dnnFormLabel dnnFormMessage dnnFormError dnnCaptcha dnnForm dnnLoginService dnnClear

Anymore community changes related to CSS classes and login please speak now I will try to help :)

@valadas
Copy link
Contributor

valadas commented Nov 10, 2019

I still think we do not need that many classes, if an element has a single label child and a single field child, we can just access it from the parent class and the exisint .dnnLabel or what have you on the child using specificity instead of adding a class name on every element on top of the already existing ones. I am saying this for 2 reasons.

  1. css files are static and better cached, etc. So I prefer having more css into the css and less in the html (just the minimum required to uniquely select something).
  2. Although this looks pretty minor, every time we add a class, it needs to stay there forever in order to not break existing themes, so this is one of the things very easy to add but very hard to remove and we should keep that at the minimum required to solve a problem.

@valadas
Copy link
Contributor

valadas commented Nov 10, 2019

And from a quick look, I don't even know if we actually have a problem to resolve here, what can't we uniquely target with css specificity currently on the login form ?

@thabaum
Copy link
Contributor Author

thabaum commented Nov 11, 2019

going forward should we use a certain naming convention for CSS that everyone agrees? I can go through and get everything ready for styling for enhancing the user experience.

Should I use

  1. class="dnn-login-form-item-username"
  2. class="dnnLoginFormItemUsername" (Current naming convention not generally recommended or used but I can work with it)
  3. class="dnn_login__form_item_username"
  4. class="login-form-item-username" (removes the dnn which I believe is best practice and most common way CSS developers are used to seeing.)

We can also shorten the common form-item to type-f-i-object/action such as login-f-i-cancel.

If you like how it is I will keep with the same that has been used. Personally I kind of like the way it is but for a CSS developer this could be a deal breaker... I would lean towards helping out the CSS developer community here totally as much as I can so any help is awesome! However the original way of doing things will still exist so it may not matter and might make things more easier to follow staying the way everyone is used to already.

Please advise.

I will go through all my PR's and change this to whatever works for everyone. Trying to make them short as well so removing the - helps. If we can agree on a naming convention for the CSS classes I believe that needs to be set before going forward. A general rule that can be broke if needed.

I have a couple more PR's similar to this nearly ready to go as well for some other user areas.

I believe everyone is in desperate need of being able to really manipulate everything in DNN as far as styles go by being able to get past some of the current limits.

These changes will have to stay around for a while I believe so please give me some feedback here. You can see what I am trying to accomplish I wish to do it in the best way for everyone. Thank you.

@mitchelsellers
Copy link
Contributor

Although you are leaving the existing styles in place, this to me seems to be really pushing the bloat on the login form, without a true tangible benefit to the platform.

I think trying to get an understanding of what cannot be styled, etc would be good, otherwise, at a minimum a more detailed review, as I think the goal could be addressed with quite a few less classes.

@valadas
Copy link
Contributor

valadas commented Nov 11, 2019

I totally agree, a good start is to find what is unstylable uniquely due to a lacking class. From a quick look I think we can already access any single html element with css specificity with the existing classes.

@thabaum
Copy link
Contributor Author

thabaum commented Nov 11, 2019

In a way everything can be styled, it was more for making it easier for newbies to style each piece of the login. Just seemed like a good idea at the time. Reality is you can do it already... just takes more thought and a higher level of skills.

I was having second thoughts as well here but I still feel classes can be added to simply things so you can just use one class to style.

And I believe a few are missing.

I would really love some time to learn more and share my results and hear a discussion on these.

I did the Journal Module it was missing a lot in my opinion to make it easy to style the entire thing. But I don't want to bloat anything for beginners or ease of use.

Everything with an ID can be uniquely accessed just not easily. You have to know more in depth how to edit those areas and for a lot of beginners this is pretty deep stuff.

Get rid of the cancel button... but when you remove it the page module fails to load. I was going to play with this line a little more so that the sign in can move all the way to the right.

Personally I would just like to see the login boxes and all text and buttons center with the name of the field labels inside the text boxes with a login on the right. I think all of these controls should pretty much be tokens for the login form and allow you to create your own login form using the HTML editor. That would allow me to create all my own CSS and everything and change when I like.

image

this page has

image

I think the DNN login should be similar in respect to login-form , login-input, login-submit and change some of theres to login-recover or something to that effect.

So what is truely broken here is the naming convention, it is todays worst practice.

A part of these efforts I figured could go toward renaming classes for DNN to follow the best practices of today. And put our DNN community twist to it.

If I can get down exactly how we can approach this I can take on the chore of making it happen.

@thabaum
Copy link
Contributor Author

thabaum commented Nov 11, 2019

I have a feeling that navigating from old ways to new ways by version 11 is the goal, with hopefully version 11 or 12 introducing .net core? So this CSS class naming convention and any other one should be reviewed for possible solutions to be best practices industry standard for each class. This means using a dash - between words instead of lowercaseUppercase. Otherwise we are not modernizing to todays standards at least.

One goal I really think should be focused on is Mobile first approach, but never forget about the power users and give some added features to them for using bigger screens such as expanding menus.

The administration panel can probably use another tweak to use less screen on mobiles.

Moving all form field labels inside of the form to get use more of the limited screens in mobile...

This is a bit of the same topic and maybe just doing some CSS tricks along with standard naming convention that is easy-to-read throughout DNN for classes could be something we can direct some time towards?

It is like an ever expanding universe, the more you know the more there is to know...

One last thing is I dont think using ID's to style css is a better practice than using a css class. I believe I read it was not ideal, however it can be done that way as well. For CSS Developers to edit on the fly they like to see unique classes.

So I suppose my proposal would include eventually moving the current CSS class to obsolete and detect any classes that have lowercaseUppercase as absolete to help detect what needs notification of being updated and removed later in v11 source.

Now that we have no parent watching over us kids we can finally play!

@mitchelsellers
Copy link
Contributor

I really appreciate the enthusiasm here, but this is approaching a much larger architecture discussion and something that if we are going to do, needs to be done across the board, not just on the login forms.

We have to consider the migration path, upgrade risks, and breaking changes of anything. Part of this continued discussion is the future of the default.css file that is distributed with the platform.

My $0.02 on this is that we close this for now, and work on a full plan of attack, that coordinates with longer-terms goals etc. And focuses on solving issues

@thabaum
Copy link
Contributor Author

thabaum commented Nov 11, 2019

How do we work on it if we close it?

@thabaum
Copy link
Contributor Author

thabaum commented Nov 11, 2019

Maybe Request for Comments?

I will put this up as I want to get this discussion going and put a post in the forums to help direct the community to it.

Can close these and I will reference the issues and these PR's in the RFC

Thank you.

@mitchelsellers
Copy link
Contributor

We can do this as a RFC, but really it needs a bit of a working group discussion before it gets there. I would want to get input from the designers, as well as developers.

We can discuss during the next TAG meeting as appropriate to try and create a team for it.

@thabaum
Copy link
Contributor Author

thabaum commented Nov 13, 2019

Thank you, I will start looking at other things I can help with until I hear word. I dont have to do it if someone else can... just trying to help if needed where I can to make DNN the best solution for my future development efforts.

I am really looking 5 years down the road with DNN. I almost had to turn away but I see there is hope so I am going to hang with it.

@thabaum thabaum closed this Nov 13, 2019
@thabaum thabaum deleted the patch-7 branch December 7, 2021 03:09
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.

3 participants