Skip to content

Ugly ➕ icon for array-item-add #337

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

Closed
mbeaudru opened this issue Oct 8, 2016 · 14 comments
Closed

Ugly ➕ icon for array-item-add #337

mbeaudru opened this issue Oct 8, 2016 · 14 comments
Labels

Comments

@mbeaudru
Copy link

mbeaudru commented Oct 8, 2016

Prerequisites

Description

Can someone explain why the normal + has been ditched for this one ? It doesn't look good to me but I might be wrong or missing something.

Commit that changes the symbol

Steps to Reproduce

See how it renders

Expected behavior

image

Actual behavior

image

Version

0.40.0

@n1k0
Copy link
Collaborator

n1k0 commented Oct 10, 2016

The thing is, the updated icon looks slightly better on my Linux laptop, but not at all like on your screenshot. And I don't know with other OSes.

I'll take some time to investigate this, thanks for the report :)

@mbeaudru
Copy link
Author

mbeaudru commented Oct 10, 2016

Oh I see ! Didn't expected that 😄

For the record, I've checked on my Android phone and it looks different, but not good:

image

Friendly parenthesis: I did let the entire screenshot in order to stress out that there is an issue with the centering of the icon on the button too. It was already there before, and I opened up an issue about that 👍

@maartenth
Copy link
Contributor

There are two problems with using emoji as icons: they look different on every system and they don't respond to text color, which is a problem with restyling. (Currently, the styling is color: #fff but the plus is black on my macOS system.)

Since the library uses bootstrap anyway, is it an idea to simply use glyphicons?

<span class="glyphicon glyphicon-plus"></span>

Also, the padding left and right should be removed to keep the icon centered on narrow screens.

@n1k0
Copy link
Collaborator

n1k0 commented Nov 2, 2016

Since the library uses bootstrap anyway, is it an idea to simply use glyphicons?

This would heavily couple this library to bootstrap, as the glyphicon font and related styles would be required to render icons. We'd like to keep the lib as agnostic as we can wrt css styling, hence why we went with emojis. Maybe we should switch back to texts?

@maartenth
Copy link
Contributor

Yes, I think the 'text plus' would look better on many systems, and would integrate better with custom styling.

This would heavily couple this library to bootstrap

I understand your point. In the long term it may be an option to split out all widgets and fields into components and containers, where containers only contain the business logic and components only dom nodes. Then components can be made for different css frameworks in different libraries, e.g. called

  • react-jsonschema-bootstrap-components for Bootstrap,
  • react-jsonschema-react-bootstrap-components for react-bootstrap,
  • and even react-native-jsonschema-components for react-native.

In angular-schema-form for example, the bootstrap templates were recently split out to a different library to decouple it completely from Bootstrap.

maartenth added a commit to TND/react-jsonschema-form that referenced this issue Nov 3, 2016
@maartenth
Copy link
Contributor

maartenth commented Nov 3, 2016

plus

I updated the icon on my fork and improved the normal '+' with a css transform to make it bigger and more in line with the other buttons. If you are OK with a css transform in the library, I will submit this as a pull request.

@n1k0
Copy link
Collaborator

n1k0 commented Nov 3, 2016

The lib doesn't ship with any css file at all, and I'd like to keep it that way.

I'm stunned we can't find a plus icon/char compatible across platforms. There must be a way :)

@maartenth
Copy link
Contributor

Yes I updated the existing inline style, I did not add a css file. :)

@n1k0
Copy link
Collaborator

n1k0 commented Nov 3, 2016

That's interesting. Could you submit a PR so I see how that looks?

@n1k0
Copy link
Collaborator

n1k0 commented Nov 4, 2016

After giving all this some thinking, I think we could go with just using glyphicons and document a way to override the way they look; if we use something like

<button type="button" class="btn btn-info btn-plus col-xs-12" tabindex="-1">
  <i class="glyphicon glyphicon-plus"/>
</button>

Then it's easy to do something in CSS like

.btn-plus > i {
  display: none;
}
.btn-plus::after {
  content: "Add";
}

What do you think?

@mbeaudru
Copy link
Author

mbeaudru commented Nov 4, 2016

I thought you didn't wanted to use glyphicons ? 😕

I'm not really sure I understand the benefits of a "plus" glyphicon against a "+" character eventually customized with css to look good.

@n1k0
Copy link
Collaborator

n1k0 commented Nov 4, 2016

I'm not really sure I understand the benefits of a "plus" glyphicon against a "+" character

Consistency across rendering platforms, easy to override, and I'm not super fond of inline styling as they've always been tricky to deal with from outside components in my experience.

But I'm still open to other suggestions!

@mbeaudru
Copy link
Author

mbeaudru commented Nov 4, 2016

As long as you are good with using glyphicons I think your proposal is just fine 👍

@n1k0
Copy link
Collaborator

n1k0 commented Nov 4, 2016

@mbeaudru @maartenth I've created #375 so we can challenge this approach.

@n1k0 n1k0 closed this as completed in 0de04c6 Nov 4, 2016
n1k0 added a commit that referenced this issue Nov 9, 2016
This release has been made possible by the combined work of @olzraiti, @maartenth, @vinhlh, @tiemevanveen , @dehli, @enjoylife, @PabloEn, @israelshirk, @sjhewitt and @rom10. Thank you folks!

Breaking changes
----------------

Support for passing `DescriptionField`, `TitleField` and `SchemaField` as `Form` props as been dropped in this release. You now have to always pass them through the `fields` prop.

Deprecations
------------

* Leverage `ui:options` everywhere (fix #370) (#378)

There's now a unique recommended way to specify options for widgets in uiSchema, which is the `ui:options` directive. Previous ways are still supported, but you'll get a warning in the console if you use them.

New features
------------

* Allow overriding the default fields/widgets (#371)
* Pass data to `FieldTemplate` as strings instead of as React components (#341)
* Pass `schema` & `uiSchema` to the field template component (#379)
* Add `ui:order` wildcard feature and improve error messages (#368)
* Add support for widget autofocus (#288)
* Array field optional sortability (#345)
* Radio widget support for integers and numbers (#325)
* Add support for inline radios and checkboxes. (fix #346) (#348)
* Added ref to `FileWidget`. (#355)
* Added `removable` and `addable` options for array items (#373)
* Enable Windows development (#320)

Enhancements and bugfixes
-------------------------

* Fix `minimum/maximum==0` for `UpDownWidget` and `RangeWidget` (#344)
* Handle numbers requiring trailing zeros with more grace (#334)
* Pass empty title to `TitleField` instead of name (#311)
* `asNumber`: return `undefined` when value is empty string (#369)
* Use [glyphicons](http://getbootstrap.com/components/#glyphicons) for buttons by default. (fix #337) (#375)
* Support old versions of React (#312)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants