Skip to content

Comments

[4.0] Custom select SVG improvements#28755

Closed
C-Lodder wants to merge 4 commits intojoomla:4.0-devfrom
C-Lodder:custom-select
Closed

[4.0] Custom select SVG improvements#28755
C-Lodder wants to merge 4 commits intojoomla:4.0-devfrom
C-Lodder:custom-select

Conversation

@C-Lodder
Copy link
Member

@C-Lodder C-Lodder commented Apr 22, 2020

Pull Request for Issue #28463

Summary of Changes

Fix the styling issues present with the current custom-select.

  • Same SVG used for both LTR and RTL.
  • Simplified styling

Testing Instructions

  1. Run node build.js --compile-css from your terminal
  2. Go to the Module Manager in the backend and open any module
  3. Have a play around with the select dropdowns
  4. Open the inspector and replace dir="ltr" with dir="rtl" on the <html> node.

Screenshot

Screenshot_2020-04-22 Menus Edit Item - Joomla 4 - Administration

@brianteeman
Copy link
Contributor

Thank you

@infograf768
Copy link
Member

Will test tomorrow.

@astridx
Copy link
Contributor

astridx commented Apr 22, 2020

Thank you. I have tested this PR and I like it and everything works as described.

ماژول ها  مسیر سایت   test   مدیریت
Modules  Breadcrumbs   test   Administration

However, I personally would prefer a green background behind a white arrow . At the moment there is a green background behind a black arrow.

Modules  Breadcrumbs   test   Administration(1)

Is this possible?

@C-Lodder
Copy link
Member Author

The styling can be argued about in someone elses PR/issue. Would like to keep this purely about the SVG issue

@infograf768
Copy link
Member

I have mixed feelings here.

  1. It keeps using a svg (even if unique) instead of icons
  2. It looks like the svg color is defined by the variable $custom-select-indicator-color which therefore would have to be overridden if we want it to be white instead of $gray-900. Is that easy to do?
  3. The border color change is hardly readable for me.

@C-Lodder
Copy link
Member Author

  1. I'm not sure what you mean by this
  2. It's easy to override the variable
  3. Additional styling such as solid backgrounds (green/red) can be done by someone else. I'm not going to argue the toss about personal styling preferences in this PR. The objective here was to replace the multiple poorly constructed SVG's with something simple.

The only thing you need to be testing is:

  1. The SVG is visible
  2. The SVG is exactly the same in RTL without the need of an additional icon
  3. Setting a background-color for the <select> element is visible on the entire element

@infograf768
Copy link
Member

It's easy to override the variable

For my info: can you give me an example of a scss that would override the variable when we need the svg to be white on a different background like Published/Unpublished (Greeen/Red)

@infograf768
Copy link
Member

Here I get
Screen Shot 2020-04-23 at 09 55 00

@C-Lodder
Copy link
Member Author

@infograf768 Oh I see what you mean. If Atum was using PostCSS, this would easily be possible with scoped variables. With SCSS, I think you'd have to create a mixin.

Again, styling changes like the select BG colour are opinion based and are not part of this PR's scope. It can be argued about somewhere else.

@infograf768
Copy link
Member

Therefore, the only solution would be to create a new variable with a white filled svg and 2 new variables to use it in ltr/rtl?

@C-Lodder
Copy link
Member Author

That would basically just make this PR pointless. You would need to extended the changes done in this and use a mixin instead.

@dgrammatiko
Copy link
Contributor

@C-Lodder the recommendation is that you shouldn't ever hardcode the fill in the svg so you can control it (class/inline css/whatever). So I guess you could remove the hardcoded bit from the svg part and add a fill: currentColor; in the customSelect class. That will cover 99.99% of the cases

@C-Lodder
Copy link
Member Author

@dgrammatiko You cannot use CSS's fill property on a background SVG.

@dgrammatiko
Copy link
Contributor

Then use css custom properties 😉

@C-Lodder
Copy link
Member Author

C-Lodder commented Apr 23, 2020

You can't do interpolation with a background url().

Using SCSS will require a mixin or a bunch of additional variables

@astridx
Copy link
Contributor

astridx commented Apr 24, 2020

You can't do interpolation with a background url().

Using SCSS will require a mixin or a bunch of additional variables

Am I correct? The main problem is that we don't use inline SVGs?

@C-Lodder
Copy link
Member Author

C-Lodder commented Apr 24, 2020

You can't use inline SVG's within a <select> element.

You've got 5 options that I can think of:

  • Move from SCSS to Post CSS to make use of locally scopped CSS variables
  • Use a SCSS mixin
  • Replace the SVG with a CSS caret
  • Go back to dirty SCSS
  • Don't use solid background (green/red)

@infograf768
Copy link
Member

is it really impossible to use icons?
<span class="fas fa-angle-down" aria-hidden="true"></span>

@C-Lodder
Copy link
Member Author

@infograf768 <select> only allows <option> or <optgroup> children

@infograf768
Copy link
Member

@ciar4n
Copy link
Contributor

ciar4n commented Apr 24, 2020

If you wish to use triangles instead chevrons then this change should be done across the entire UI, something well beyond the intended scope of this PR.

@ciar4n
Copy link
Contributor

ciar4n commented Apr 24, 2020

I have tested this item ✅ successfully on 03816b9


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28755.

@astridx
Copy link
Contributor

astridx commented Apr 24, 2020

I was just about to see how everything could be switched to FontAwesome. (I just wanted to get some experience.) And now I believe that we should first standardize the HTML markup.

There are many different ways in which the select fields are inserted.

@C-Lodder When integrating with choices, your PR no longer shows an icon.

Modules  Breadcrumbs   test   Administration(2)

I don't understand why position is integrated with choices and status doesn't. Am I missing something? Both are single selects.

What do you think: shouldn't we first standardize the basis (HTML) and then build the CSS (SCSS) cleanly on it?

Regardless of whether we use (inline) SVG or FontAwesome.

@C-Lodder
Copy link
Member Author

@astridx ChoicesJS styling is fixed.

The HTML is already standardised. It's just a simple <select> dropdown with options.

Probably just easier to remove the custom-select class from all elements and let the browser use it's own icon.

@astridx
Copy link
Contributor

astridx commented Apr 24, 2020

@C-Lodder
The HTML is already standardised. It's just a simple <select> dropdown with options.

Standardizes is a relative expression :)

Why

for position

<div class="control-group">
	<div class="control-label"><label id="jform_position-lbl" for="jform_position">
	Position</label>
</div>
	<div class="controls">
		<joomla-field-fancy-select class="" allow-custom="" search-placeholder="Type or select some options"><div class="choices" data-type="select-one" tabindex="0" role="combobox" aria-autocomplete="list" aria-haspopup="true" aria-expanded="false"><div class="choices__inner"><select id="jform_position" name="jform[position]" class="choices__input" tabindex="-1" data-choice="active" hidden=""><option value="breadcrumbs">breadcrumbs</option></select><div class="choices__list choices__list--single"><div class="choices__item choices__item--selectable" data-item="" data-id="1" data-value="breadcrumbs" data-custom-properties="null" aria-selected="true" data-deletable="">breadcrumbs<button type="button" class="choices__button_joomla" aria-label="Remove item: 'breadcrumbs'" data-button="">Remove item</button></div></div></div><div class="choices__list choices__list--dropdown" aria-expanded="false"><input type="text" class="choices__input choices__input--cloned" autocomplete="off" spellcheck="false" role="textbox" aria-autocomplete="list" aria-label=":: None ::" placeholder="Type or select some options"><div class="choices__list" role="listbox"><div class="choices__group " role="group" data-group="" data-id="841734803318" data-value=""><div class="choices__heading"></div></div><div id="choices--jform_position-item-choice-1" class="choices__item choices__item--choice choices__item--selectable is-highlighted" role="treeitem" data-choice="" data-id="1" data-value="" data-select-text="Press to select" data-choice-selectable="" aria-selected="true">:: None ::</div><div class="choices__group " role="group" data-group="" data-id="784749372895" data-value="Cassiopeia"><div class="choices__heading">Cassiopeia</div></div><div id="choices--jform_position-item-choice-2" class="choices__item choices__item--choice choices__item--selectable" role="treeitem" data-choice="" data-id="2" data-value="banner" data-select-text="Press to select" data-choice-selectable="">Banner [banner]</div><div id="choices--jform_position-item-choice-3" class="choices__item choices__item--choice choices__item--selectable" role="treeitem" data-choice="" data-id="3" data-value="bottom-a" data-select-text="Press to select" data-choice-selectable="">Bottom-a [bottom-a]</div><div id="choices--jform_position-item-choice-4" class="choices__item choices__item--choice choices__item--selectable" role="treeitem" data-choice="" data-id="4" data-value="bottom-b" data-select-text="Press to select" data-choice-selectable="">Bottom-b [bottom-b]</div><div id="choices--jform_position-item-choice-5" class="choices__item choices__item--choice choices__item--selectable" role="treeitem" data-choice="" data-id="5" data-value="breadcrumbs" data-select-text="Press to select" data-choice-selectable="">Breadcrumbs [breadcrumbs]</div><div id="choices--jform_position-item-choice-6" class="choices__item choices__item--choice choices__item--selectable" role="treeitem" data-choice="" data-id="6" data-value="debug" data-select-text="Press to select" data-choice-selectable="">Debug [debug]</div><div id="choices--jform_position-item-choice-7" class="choices__item choices__item--choice choices__item--selectable" role="treeitem" data-choice="" data-id="7" data-value="footer" data-select-text="Press to select" data-choice-selectable="">Footer [footer]</div><div id="choices--jform_position-item-choice-8" class="choices__item choices__item--choice choices__item--selectable" role="treeitem" data-choice="" data-id="8" data-value="main-bottom" data-select-text="Press to select" data-choice-selectable="">Main-bottom [main-bottom]</div><div id="choices--jform_position-item-choice-9" class="choices__item choices__item--choice choices__item--selectable" role="treeitem" data-choice="" data-id="9" data-value="main-top" data-select-text="Press to select" data-choice-selectable="">Main-top [main-top]</div><div id="choices--jform_position-item-choice-10" class="choices__item choices__item--choice choices__item--selectable" role="treeitem" data-choice="" data-id="10" data-value="menu" data-select-text="Press to select" data-choice-selectable="">Menu [menu]</div><div id="choices--jform_position-item-choice-11" class="choices__item choices__item--choice choices__item--selectable" role="treeitem" data-choice="" data-id="11" data-value="search" data-select-text="Press to select" data-choice-selectable="">Search [search]</div><div id="choices--jform_position-item-choice-12" class="choices__item choices__item--choice choices__item--selectable" role="treeitem" data-choice="" data-id="12" data-value="sidebar-left" data-select-text="Press to select" data-choice-selectable="">Sidebar-left [sidebar-left]</div><div id="choices--jform_position-item-choice-13" class="choices__item choices__item--choice choices__item--selectable" role="treeitem" data-choice="" data-id="13" data-value="sidebar-right" data-select-text="Press to select" data-choice-selectable="">Sidebar-right [sidebar-right]</div><div id="choices--jform_position-item-choice-14" class="choices__item choices__item--choice choices__item--selectable" role="treeitem" data-choice="" data-id="14" data-value="top-a" data-select-text="Press to select" data-choice-selectable="">Top-a [top-a]</div><div id="choices--jform_position-item-choice-15" class="choices__item choices__item--choice choices__item--selectable" role="treeitem" data-choice="" data-id="15" data-value="top-b" data-select-text="Press to select" data-choice-selectable="">Top-b [top-b]</div><div class="choices__group " role="group" data-group="" data-id="419122872835" data-value="Active Positions"><div class="choices__heading">Active Positions</div></div><div id="choices--jform_position-item-choice-16" class="choices__item choices__item--choice is-selected choices__item--selectable" role="treeitem" data-choice="" data-id="16" data-value="breadcrumbs" data-select-text="Press to select" data-choice-selectable="">breadcrumbs</div><div id="choices--jform_position-item-choice-17" class="choices__item choices__item--choice choices__item--selectable" role="treeitem" data-choice="" data-id="17" data-value="sidebar-right" data-select-text="Press to select" data-choice-selectable="">sidebar-right</div></div></div></div>
</joomla-field-fancy-select>
</div>
</div>

and

for published

<div class="control-group">
	<div class="control-label"><label id="jform_published-lbl" for="jform_published">
	Status</label>
</div>
	<div class="controls has-success">
		<select id="jform_published" name="jform[published]" class="custom-select custom-select-color-state custom-select-success valid form-control-success" size="1" aria-invalid="false">
	<option value="1" selected="selected">Published</option>
	<option value="0">Unpublished</option>
	<option value="-2">Trashed</option>
</select>
			</div>
</div>

Because of these differences (in one case joomla-field-fancy-select and in the other case not), it happened in my tests that the icon at position disappeared - just like yours in this PR. I expected that a select in which only one selection is possible would be the same everywhere.

@C-Lodder C-Lodder changed the title [4.0] Custom select styling improvements [4.0] Custom select SVG improvements Apr 24, 2020
@brianteeman
Copy link
Contributor

@astridx choicesJS is used where we need to select multiple AND/OR where we need to create on the fly eg a category or position

@astridx
Copy link
Contributor

astridx commented Apr 24, 2020

Thanks @brianteeman, I haven't seen that before. No I understand: Position is created with choicesJs because you can create position.

@C-Lodder C-Lodder closed this Apr 25, 2020
@C-Lodder C-Lodder deleted the custom-select branch April 25, 2020 14:00
@dgrammatiko
Copy link
Contributor

@C-Lodder mate why did you close this one?
The idea to have a dependency on Font Awesome even for the simplest HTML elements is making me extremely sad...

@C-Lodder
Copy link
Member Author

Closing as it's being done here: #28787

@dgrammatiko
Copy link
Contributor

@C-Lodder I saw it: hardcoding Font Awesome to all the select elements? What could possible go wrong there...

@Quy
Copy link
Contributor

Quy commented Jul 19, 2020

@C-Lodder #28787 is closed. Please reconsider opening this PR. Thank you!

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.

8 participants