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

Number with currency widget #1793

Merged
merged 14 commits into from
Mar 30, 2017
Merged

Conversation

jhawthorn
Copy link
Contributor

@jhawthorn jhawthorn commented Mar 22, 2017

cc @Mandily @tvdeyen

I would like to introduce a "Number with currency" widget to use on every form with a monetary amount.

Currently whenever we are looking to input a monetary value, we just have a plain text box for the amount, and a second field for the currency (if it can be changed by that form).

This widget combines both inputs. It comes in two flavours, either with a dropdown for the currency (if it is changeable here) or fixed (where we previously didn't show a currency). I think it is better to always show the currency, both to make it obvious what currency is being used and for consistency.

Examples

Product edit

After Before

Promotion Rules and calculators

After

Before

Payment

After
(layout and padding could use some fixing here before the final version)

Before

Try it

To try it out you can clone this down or try a demo on codepen

Implementation

This depends on having the font size match bootstrap's, which @tvdeyen has been working on. The widget uses bootstrap's styling of input addons, for the basic layout.

The selector (when enabled) is a standard HTML <select> (not select2) with custom styling to make it blend in better. I've done some cross browser testing, and what I have now seems to function well on chrome/FF/IE11/safari.

There's a small backbone.js view which watches for changes to the currency so it can update the symbol.

@Senjai
Copy link
Contributor

Senjai commented Mar 22, 2017

😍 👍

@Mandily
Copy link
Contributor

Mandily commented Mar 22, 2017

I like this a lot. I'm assuming we'd use the same thing in the settings where you set the master currency for each store?

@jhawthorn
Copy link
Contributor Author

I like this a lot. I'm assuming we'd use the same thing in the settings where you set the master currency for each store?

This probably only makes sense when entering an amount.

Copy link
Contributor

@Sinetheta Sinetheta left a comment

Choose a reason for hiding this comment

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

Now that we have components inheriting styles from $input-color (instead of catching the old solidus styles) we should probably replace that boostrap default with the current solidus admin color

# backend/_bootstrap_custom.scss
-$input-color:                    $gray !default;
+$input-color:                    $brand-primary;

background: url("data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' width='1024' height='640'><path d='M1017 68L541 626q-11 12-26 12t-26-12L13 68Q-3 49 6 24.5T39 0h952q24 0 33 24.5t-7 43.5z'></path></svg>") 90%/12px 6px no-repeat;

font-family: inherit;
color: inherit;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason for these inherits? That would be the default and it's also in our reset.

-moz-appearance:none;
appearance:none;

background: url("data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' width='1024' height='640'><path d='M1017 68L541 626q-11 12-26 12t-26-12L13 68Q-3 49 6 24.5T39 0h952q24 0 33 24.5t-7 43.5z'></path></svg>") 90%/12px 6px no-repeat;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what way you want to go here, but you could also add <i class="fa fa-caret-down" aria-hidden="true"></i> and position as necessary.

The svg us much simpler, but harder to color/theme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't be done. This is the background of a <select>

Copy link
Member

Choose a reason for hiding this comment

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

There is also asset-data-url

https://github.com/rails/sass-rails#asset-data-urlrelative-asset-path

then we can make this changeable and it's still a data uri.

Copy link
Contributor

@mtomov mtomov Mar 24, 2017

Choose a reason for hiding this comment

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

another option might be to use ::before / ::after selectors and content: property with the encoded symbol (not entirely sure what the svg is) as a value and position it absolutely, and set the font-family to the one for the icons, and finally give the element a position: relative

this.$('.currency-selector-symbol').text("");
}
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

We can break this up a bit

Spree.Views.NumberWithCurrency = Backbone.View.extend({
  events: {
    'change input,select': "render"
  },

  initialize: function() {
    this.defaultCurrency = this.$('[data-currency]').data("currency");
  },

  getCurrency: function() {
    return this.$('.currency-selector').val() || this.defaultCurrency;
  },

  updateSymbol: function(currency) {
    var symbol = '';
    if (currency) {
      symbol = Spree.currencyInfo[currency][0];
    }
    this.$('.currency-selector-symbol').text(symbol)
  },

  render: function() {
    var currency = this.getCurrency();
    this.updateSymbol(currency);
  }
});

@Sinetheta
Copy link
Contributor

Sorry about the nits, I didn't read the PR message. Looks great as a UI @jhawthorn 👍

@tvdeyen tvdeyen added WIP changelog:solidus_backend Changes to the solidus_backend gem labels Mar 23, 2017
@tvdeyen
Copy link
Member

tvdeyen commented Mar 23, 2017

I like this a lot. There is still room for improving, though. For me it's hard to distinguish between changeable and "not changeable" currency. The only differentiation is the little black arrow. Maybe we could remove the background from the "display only" or use a different bg colour? Not sure.

Thanks John! 🍺

color: transparent;
text-shadow: 0 0 0 #000;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

can we have a:

&:hover {
  cursor: pointer;
}

here please?

Copy link
Contributor Author

@jhawthorn jhawthorn Mar 23, 2017

Choose a reason for hiding this comment

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

👍

I'm going to try &:hover:not(:active). When testing with just :hover it felt a little weird. When opening the select, the cursor changed from pointer to arrow and then quickly back. Using :hover:not(:active), it just switches to arrow, and stays that way while open. This is the same behaviour we get from the select2 controlled selects.

Copy link
Member

Choose a reason for hiding this comment

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

👌🏻

@jhawthorn
Copy link
Contributor Author

@tvdeyen to help differentiate between selectable and fixed currencies, I'm going to try make the background white when it is a select box. If we also go with the change I propose in #1797 "Replace select2 styling" it should match other select boxes and be even more obvious.

Also allows store credits to be created with a currency other than the
default.
Instead of entirely styling from scratch, we try to bend the browser's
internal styles to match. This avoids needing an SVG background for the
select's dropdown, and matchces bootstrap's styling of select boxes.
Originally I had named the component "currency-selector", but that
doesn't do a good job of describing the whole component.
@jhawthorn jhawthorn changed the title [WIP] Number with currency widget Number with currency widget Mar 28, 2017
@jhawthorn
Copy link
Contributor Author

jhawthorn commented Mar 28, 2017

This is no longer WIP and is good to go.

Here's how the selectable vs fixed currency looks now.

@jhawthorn
Copy link
Contributor Author

jhawthorn commented Mar 28, 2017

At @tvdeyen's suggestion I've swapped this to use bootstrap's custom select styling, which has the advantage of looking (more or less) identical across browsers and reduces the CSS we need to achieve this.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Really like this 👍

I have some comments about removing styles already set by Bootstrap.

Also I think we should add tests for the changes in the controllers.

select {
@extend .c-select;

display: inline-block;
Copy link
Member

Choose a reason for hiding this comment

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

display: inline-block is already set by Bootstrap in .c-select


&::-ms-expand {
// Required to see full text of selected value on IE11
display: none;
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

opacity: 0 didn't do the trick on IE11. The "D" in "USD" was cut off, despite there being lots of room in the select box. See comment.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting. Ok. Thanks for clarification.


display: inline-block;
width: 100%;
height: 31px;
Copy link
Member

Choose a reason for hiding this comment

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

Can we use Bootstraps $input-heighthere?

Copy link
Contributor Author

@jhawthorn jhawthorn Mar 29, 2017

Choose a reason for hiding this comment

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

👍

This leaves a 1px gap on the top and bottom (height is 2.25rem == 29px), but I don't think that's a huge issue, and is worth it to avoid this magic number and ensure the input will scale well if our font size changes.


& &-select {
padding: 0;
background: #fff;
Copy link
Member

Choose a reason for hiding this comment

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

Let's make use of Bootstraps $input-bg here.

.number-with-currency {
&-symbol {
min-width: 2.5em;
text-align: center;
Copy link
Member

Choose a reason for hiding this comment

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

text-align: center; is already set by Bootstraps .input-group-addon

@jhawthorn
Copy link
Contributor Author

@tvdeyen Thanks. I have addressed your feedback.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Great stuff. 🌟

@jhawthorn jhawthorn removed the WIP label Mar 30, 2017
@jhawthorn jhawthorn merged commit 54a4980 into solidusio:master Mar 30, 2017
@mtomov
Copy link
Contributor

mtomov commented Mar 31, 2017

Just played with this for a bit, and it's super cool! The up and down keys from the keyboard work just as well, and the immediate feedback that you get with the currency sign change in front is awesome! Very nice piece of UI! Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_backend Changes to the solidus_backend gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants