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

NumericField Spinner: replaced jQuery UI with Bootstrap components & … #7421

Merged
merged 1 commit into from
Oct 28, 2020

Conversation

netwavebe
Copy link
Contributor

@netwavebe netwavebe commented Oct 28, 2020

…added support for scale > 0 with localization

The old implementation, based on jQuery UI, had some issues. It was not responsive, it added unnecessary style classes and it did not support configured min/max values. Furthermore, it did not support scale > 0 at all.

I created a content type with some numeric fields. The hint shows how they are configured.

The old spinner in action:

SpinnerBefore

I removed the jQuery UI dependency and replaced it with Bootstrap components. Styling and responsive behaviour is now in line with other fields. Values are also kept within the min/max range.

The new spinner in action:

SpinnerAfter

Support for scale > 0 was also added. For this I had to check the decimal separator that is used on the server and optionally convert commas to points and vice versa. Look at the sourcecode how I implemented this. If there is a better way, I'd like to know.

…added support for scale > 0 with localisation
@netwavebe
Copy link
Contributor Author

#2266 discusses the problems the old spinner had.

<div class="input-group-append">
<div class="input-group-text">@max</div>
<button class="btn btn-outline-secondary" type="button" onclick="numericFieldSpinner('@(id)', @(scale), -@(step), @(min), @(max))"><i class="fa fa-minus" aria-hidden="true"></i></button>
Copy link
Contributor

@Skrypt Skrypt Oct 28, 2020

Choose a reason for hiding this comment

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

Could you try with btn-outline-light and show me a screenshot without commiting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

They need to add one that matches the form input border color. 😢

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. btn-outline-secondary was visually the best (probably why it's used on https://getbootstrap.com/docs/4.0/components/input-group/#button-addons as a demo).

});
});

<script asp-name="numericFieldSpinner" at="Foot">
Copy link
Contributor

@Skrypt Skrypt Oct 28, 2020

Choose a reason for hiding this comment

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

I don't think name is allowed on a script tag. It is not semantically correct. It should be used only when referencing a script from the ResourceManager. It should not render anything in the HTML, it's fine, but then you can also remove. 😄

Copy link
Contributor Author

@netwavebe netwavebe Oct 28, 2020

Choose a reason for hiding this comment

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

It's a server-side thing that does not get rendered to the client. Naming a script prevents the same script from rendering multiple times. If I remove this, the script is rendered for each spinner on the page. Naming the script prevents this: it's only rendered once for all spinners.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahhh I see LGTM then.

@Skrypt Skrypt self-requested a review October 28, 2020 20:02
@agriffard agriffard merged commit c42706b into OrchardCMS:dev Oct 28, 2020
@netwavebe netwavebe deleted the pr-numericfield-spinner branch October 28, 2020 21:29
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