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

Review multiple class selection when there are many classes #786

Merged
merged 3 commits into from
Jul 9, 2018

Conversation

rodfersou
Copy link
Member

@rodfersou rodfersou commented Jun 25, 2018

closes #785

@rodfersou rodfersou requested a review from hvelarde June 25, 2018 19:15
@hvelarde hvelarde requested a review from fredvd June 25, 2018 19:20
CHANGES.rst Outdated
@@ -6,7 +6,8 @@ There's a frood who really knows where his towel is.
1.7b3 (unreleased)
^^^^^^^^^^^^^^^^^^

- Nothing changed yet.
- Review multiple class selection when there is many classes (closes `#785 <https://github.com/collective/collective.cover/issues/785>`_).
Copy link
Member

Choose a reason for hiding this comment

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

when there are many classes.

@hvelarde hvelarde changed the title Review multiple class selection when there is many classes Review multiple class selection when there are many classes Jun 25, 2018
@rodfersou rodfersou force-pushed the review-multiple-class-selection branch from a207124 to fe937b0 Compare June 25, 2018 20:44
@hvelarde hvelarde force-pushed the review-multiple-class-selection branch from fe937b0 to 7aa7884 Compare June 26, 2018 18:56
@hvelarde
Copy link
Member

@fredvd would you mind to review this?

@hvelarde hvelarde force-pushed the review-multiple-class-selection branch from 7aa7884 to 31a77b4 Compare June 27, 2018 21:28
@hvelarde
Copy link
Member

@fredvd any feedback?

@fredvd
Copy link
Member

fredvd commented Jun 28, 2018

@hvelarde. I’m on vacation this and next week, I’ll take a look and test the fix tomorrow morning. Thank you for looking into this!

@fredvd
Copy link
Member

fredvd commented Jun 29, 2018

@rodfersou I've checked out the branch in a local project, the updated css is pulled in, but I don't see any difference in behavior in the css class dropdown:

css-class-dropdown

Edit: I wanted to check on the c.cover buildout in plain Plone as well but got cut off by takeoff from Amsterdam to München for Pyconweb :-). On mobile now, I’ll check that first this evening

@rodfersou
Copy link
Member Author

@fredvd this is weird.. are you sure you are using this branch version of Cover?

@rodfersou
Copy link
Member Author

@hvelarde done

@fredvd
Copy link
Member

fredvd commented Jun 30, 2018

Reviewed on Plain Plone/collective.cover buildout . The problem is fixed from a visual point of view, as I feared/suspected my own project is overriding some css probably.

There still is a usability problem now: the drop down selector list doesn't show the scroll bar by default at least in Chrome, so you don't know there are more options and you can click one option and close: until you are able activate scrolling some way or another in the area (by selecting text and then click scroll, which is non-obvious).

(this is default Plone with 15 css tile classes configured):

class-selection_popup-2

@rodfersou
Copy link
Member Author

@fredvd here it works fine
image
I'll take another look in the css..

@fredvd
Copy link
Member

fredvd commented Jul 2, 2018

@rodfersou this was on chrome/mac with 15 configured css classes, only the first 3 show without the scrollbar present on drop down.

@rodfersou
Copy link
Member Author

@fredvd it is a bug related to macos; my last commit fixes it.

@fredvd
Copy link
Member

fredvd commented Jul 2, 2018

@rodfersou Testing this at the airport back home now, checked all major browsers on mac (chrome, safari, firefox) just to be sure. You won't believe this: all browsers except for Firefox work now: but with Firefox the scroll bar shows, and then >disappears< after 2-3 seconds. checked firefox devel and firefox stable release 52.9.0. :-(

firefox-mac

Freaking weird. But you can see the scrollbar now when you open the select dropdown and I do get a cross/arrow in the drop down area now so there's more hints there's something. Lets keep it like this and merge/close, safari & chrome work. Pfew, the old browser incompatibilities from 10 years ago are back in full swing. :-S

Where is this multi-select widget coming from, some library, or created by yourself?

@rodfersou
Copy link
Member Author

@fredvd this one is home made, we plan to use a widget after add webpack in cover buildout.

@rodfersou
Copy link
Member Author

@fredvd the disapear behavior happen just on Firefox at Macos; I change a bit the CSS and believe that it is fixed now.. can you test again please?

@fredvd
Copy link
Member

fredvd commented Jul 9, 2018

@rodfersou Back at work. Tested on Firefox mac, works fine now, the scrollbar stays. Tested on all platforms I have access to locally (mac firefox/safari/chrome), (win10: ie11, edge, chrome). Small glitch I saw is that Win-10 IE11 turns the scrollbar into two big arrows to scroll through the list can't remember if it was different before).

But it's totally workable, lets wait for webpack and use an available existing widget where others have done the hard work of fixing browser compatibility. I Never knew a dropdown list could be this complex to function browser independent.

dropdown_win10-ie11

Copy link
Member

@fredvd fredvd left a comment

Choose a reason for hiding this comment

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

Drop down list works now in the major browsers I've tested.

Pitty is that we have to keep the drop down list height small because css class select overlay for columns and has a small height, it's the only field. If those overlays could get a min-height then we could show maybe 5-6 rows by default in de css drop down instead of 3, but lets wait for webpack and a generic multi select drop down widget first instead of optimising this custom widget.

@hvelarde hvelarde merged commit 7e50ed1 into master Jul 9, 2018
@hvelarde hvelarde deleted the review-multiple-class-selection branch July 9, 2018 21:11
@hvelarde
Copy link
Member

hvelarde commented Jul 9, 2018

@fredvd thanks! do you need now a release for this?

@fredvd
Copy link
Member

fredvd commented Jul 9, 2018

@hvelarde For me it's not super urgent, but others might run into issues with the css drop down as well if they update to 1.7b2 so if no other updates/fixes are planned shortly, better to do a release.

@hvelarde
Copy link
Member

hvelarde commented Jul 9, 2018

I made a new release: https://pypi.org/project/collective.cover/1.7b3/

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.

UI issue: Multiple class select on tiles feature from 1.7b1 doesn't scale beyond 15 css classes
3 participants