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

Fix validation classes when using b:formGroup #689

Merged
merged 3 commits into from
Apr 10, 2017

Conversation

ggam
Copy link
Collaborator

@ggam ggam commented Apr 8, 2017

Still needs more testing.

Known issues (some of them not related to this PR, but I'll try to address them here anyway). They are all related to inline forms:

  • Responsive classes should be ignored on inline forms as they misalign components. Example components where the error is visible:
    • selectOneMenu
    • dateTimePicker
    • selectBooleanCheckbox
  • Some components don't add the control-label class to labels on inline forms. has-error has no effect on them. Examples:
    • selectManyMenu
  • datePicker is not correctly shown on inline forms (probably because of one of the above mention errors).

Additionaly, since datepicker has no renderer (it renders itself), I made a little hack there and put the instanceof to check if the parent component is a formGroup. I'll think of a better approach when everything else is working.

I've also finished fixing the colorPicker not rendering label by default.

@ggam
Copy link
Collaborator Author

ggam commented Apr 9, 2017

I think I've fixed all the above mentioned problems:

  • All the labels now have the "control-label" class
  • Inputs and labels inside inline-forms don't render responsive classes. I've made a really ugly check here (see Responsive#shouldRenderResponsiveClasses method), but I think it will work for now.

I'll test it all once more at the end of the day, but it will help is someone else can review the code.

@ggam ggam changed the title [WIP] Fix validation classes when using b:formGroup Fix validation classes when using b:formGroup Apr 9, 2017
@stephanrauh
Copy link
Collaborator

@ggam Yesterday, I've started to add the Bootstrap validation classes to the sliders, but truth to tell, I don't know how to do it. Adding control-label was simple. Do we want to add a CSS class to the slider component itself to indicate the validation status?

@ggam
Copy link
Collaborator Author

ggam commented Apr 9, 2017

Is Slider a Bootstrap "plugin"? Is there a possibility to ask the author how is it supposed to work?

@@ -146,7 +144,7 @@ protected void addLabel(ResponseWriter rw, String clientId, SelectBooleanCheckbo
String label = selectBooleanCheckbox.getLabel();
if (label != null) {
rw.startElement("label", selectBooleanCheckbox);
generateErrorAndRequiredClassForLabels(selectBooleanCheckbox, rw, clientId, selectBooleanCheckbox.getLabelStyleClass());
generateErrorAndRequiredClass(selectBooleanCheckbox, rw, clientId, selectBooleanCheckbox.getLabelStyleClass(), Responsive.getResponsiveLabelClass(selectBooleanCheckbox), "control-label");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've stumbled over this yesterday, too. I guess the code is correct. But can you explain how it works?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be honest, I have to admit that I don't know. I copied it from another component where it worked.

I know that's not the way to do things, but my priority for now is to fix our markup for 1.1.0. When this is merged, I plan to review all the code and propose a simpler and more unified solution, where cumbersome methods like this would dissapear.

I'm currently working on fixing the responsive classes for the components where it dind't work yet. After that I guess I could start to review code to potentially refactor.

@stephanrauh
Copy link
Collaborator

In general, the PR looks OK to me. I've left one comment at a line I didn't understand. It's almost certainly correct, but I'd like to know why.

The rest is testing, testing, testing :). Is the PR ready to be merged?

@stephanrauh
Copy link
Collaborator

stephanrauh commented Apr 9, 2017

As for the slider: I've opened an issue.
seiyria/bootstrap-slider#741

@stephanrauh
Copy link
Collaborator

I've already got an answer. Let's start with coloring the border red if the slider is in an erroneous form-group. Something like this HTML code:

<div class="form-group has-error">
  <label class="control-label" for="age">Please enter your age (18-65)</label>
  <input id="age" data-slider-id='ex1Slider' type="text" data-slider-min="0" 
    data-slider-max="99" data-slider-step="1" data-slider-value="14"/>
</div>l

@ggam
Copy link
Collaborator Author

ggam commented Apr 10, 2017

Work on this PR is finished. It just lacks proper testing. If you are ok with that, you could merge it now and then we all will test as we develop. And after that, a more torough testing should be done also as with everything else.

From what I see from the slider, the idea is to do it like on all the other inputs no? Would you like me to add it t this PR befpre merging?

@stephanrauh stephanrauh merged commit 6129d60 into TheCoder4eu:master Apr 10, 2017
@stephanrauh
Copy link
Collaborator

Yes, the idea with the slider is to provide the same API as for the other input widgets. Not sure that idea makes sense. But that's why we discuss it - to find out whether it makes sense or not, and how to do it.

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