Skip to content
This repository has been archived by the owner on Jul 3, 2023. It is now read-only.

Reengineered autotag #37

Merged
merged 24 commits into from
Apr 19, 2016
Merged

Reengineered autotag #37

merged 24 commits into from
Apr 19, 2016

Conversation

dpwrussell
Copy link
Collaborator

Completely redeveloped autotag using React.

@jburel
Copy link
Contributor

jburel commented Dec 27, 2015

Note for @hflynn:
When a version of webtagging including this PR is release:
The following page
https://www.openmicroscopy.org/site/support/partner/omero.webtagging
will have to be updated to indicate the compatible version of omero i.e. webtagging 1.3.0 support for 5.1.x, 5.2.x
and for webtagging version 1.4.0 omero 5.2.x only

@hflynn
Copy link

hflynn commented Jan 5, 2016

We don't have a downloads page for 1.4.0 as yet (cc @sbesson), probably best to sort that out before I update the product page

@jburel
Copy link
Contributor

jburel commented Jan 5, 2016

@hflynn: my comment was more a heads-up since there is no release yet

@hflynn
Copy link

hflynn commented Jan 5, 2016

Ah sorry! Gotcha.

Adds occurence based filtering ability in replacement of ignore
first/last
Massive performance upgrade
Client-side templating instead of server-side
@pwalczysko
Copy link

Tested on centos6_py27_apache24 along the lines of the new scenario https://github.com/openmicroscopy/ome-internal/pull/290. Works as expected. Note that new issues were created with bug reports as well as RFEs. #47 #46 ... - #42

@pwalczysko
Copy link

Unfortunately, the issue #36 is not resolved in the setup reported in the previous comment #37 (comment) on Firefox 43.0.4

@pwalczysko
Copy link

#25 is resolved here.

@pwalczysko
Copy link

Will have to ask @will-moore about #29

@pwalczysko
Copy link

BUG: ?
There is a complete change in behavour with regard to tokenizing in some cases compared with previous behaviour (without this PR).
Compare the two screenshots below. The names like Screen [Shot_2016-01-20 at 18.23.47.png will get tokenized in the old version on the underscore and on the first period (dot) in the name, whereas in the new version with this PR in, this is completely ignored.

Old, without this PR
screen shot 2016-01-22 at 14 58 51

New, with this PR
screen shot 2016-01-22 at 14 57 29

@pwalczysko
Copy link

@dpwrussell : in more general terms Re #37 (comment) - what tokenizing behaviour is actually the expected behaviour as of now ? (slashes, underscores .... anything else ?, what is the behaviour when a pure-number group is in the name ? I think this should be spelled out in order to know what is expected and what not.

@dpwrussell
Copy link
Collaborator Author

@pwalczysko Until I address German-BioImaging/omero-autotag#4 I have made it so that the default is to split on any of:

  • Whitespace
  • Underscore
  • Period
  • Forwardslash
  • Backslash

@dpwrussell
Copy link
Collaborator Author

For this PR to progress the remaining issues are #52 and #51.

@will-moore If you have any ideas about #43 that would also be welcome.

Also changed entire button to highlight on mouseover
Removed highlight when mouseing over the down arrow as this no longer
makes sense
Moved slider count to the left
Fixed slider allignment

Fixes #51 and #52
@will-moore
Copy link
Contributor

I just found that the bundle.js is breaking jQuery tooltip plugins in the webclient.
To test, uninstall autotag or use the 1.3.0 release and webclient tooltips (E.g. on tags in right panel) work OK. But not with this branch. Webclient currently uses $.tooltip from jQuery-ui 1.10.4.
but bundle.js overrides this with it's own $.tooltip with is not compatible.

@pwalczysko
Copy link

Bug: the formatting of the dropdown menu on the blue and pink buttons. See screenshot below. The menu is "squished" into the precise size of the buton.

screen shot 2016-02-11 at 12 22 31
screen shot 2016-02-11 at 12 22 21

@pwalczysko
Copy link

The tooltips are okay now in Web, but #37 (comment) persists.

@dpwrussell
Copy link
Collaborator Author

Ok, I think I've fixed most of those issues.

Not sure about the one with the squished menus, there are CSS rules in place to avoid that specifically. Is this happening for you on all browsers?

@pwalczysko
Copy link

Squished menus problem is fixed as of today on all browsers (see screenshot below for Safari, similarly for all browsers, the menu is now blue, and fully highlighted, not squished).
screen shot 2016-02-12 at 10 31 53

Note though that Firefox 42 on Win as well as Firefox 43 on Mac ( but not Chrome, not Safari, not IE 11) have strange color scheme on the token suggestions (originally pink buttons are now blue, originally blue ones are white, the formerly green ones are now light blue).
screen shot 2016-02-12 at 10 42 45

IE 11 on Win 7 has its own issue (not seen anywhere else) with wrong formatting of the slider (partially hidden under the central pane)
webtagging ie 11

@pwalczysko
Copy link

The Filtering images box fromat #37 (comment) is fixed now.

@pwalczysko
Copy link

The blocking problems on Safari and IE 11 #37 (comment) and #37 (comment) are fixed now

@pwalczysko
Copy link

Tooltips in web work as expected.

@pwalczysko
Copy link

In summary from today's testing:

Suggestion: have one more look at the Firefox colors issue, this might confuse users a lot, taking into account the popularity of Firefox (issue both on Mac and Win in 2 different Firefox versions)

@dpwrussell
Copy link
Collaborator Author

I have fixed the Firefox colouring problem.

I'm not sure what to do about the IE range slider. I don't actually have any windows infrastructure right now to even try this on. Any chance you could have a play @will-moore and see if there is anything we can do that would fix it easily? A CSS rule for height/padding of the IE range slider maybe.

@pwalczysko
Copy link

@dpwrussell : The Firefox colouring problem is okay now both on Mac as well as Windows.

The IE range slider problem - @will-moore suggested to put onto the element something like margin-top: -5px; where the ``-5is a good compromise over all browsers - we checked that it is working between Firefox and IE11 with-5`.

@dpwrussell
Copy link
Collaborator Author

I tried that, but it looks rather messy. I've tried doubling the toolbar height instead. Because it lines up with the bottom of the tree toolbar it still looks tidy, but now leaves plenty of room for a) the IE slider, and b) future extra controls (like the separator customizations). I had to fiddle a few other things around in order to do that. How's that look?

@pwalczysko
Copy link

IE11 on Win7:
ie11 webtagging linup

Firefox on Win7:
firefox on win7

Chrome on Win7
chrome win7

Safari on Mac
screen shot 2016-02-23 at 19 46 54

So I think it looks good.

@pwalczysko
Copy link

Bug: the slider does not perform in some situations (Safari only, all other browsers okay)

  • take the slider butoon in any position, grabbing the button with your mouse and dragging with the mouse-button down
  • drag it to the extreme left as fast as possible, then release the mouse button
  • observe that the previous number is "stuck" on the extreme left position, i.e. when dragging from for example position 9, then now also the extreme left position shows 9 (instead of 1 as expected) and so does the central pane follow suit
    • if you cannot see the bug the first time, keep dragging the slider button from extreme left to extreme right and vice versa
    • note that when you move the slider button by clicking into the slider position, this bug does not appear

…ge. This is also extremely inconsistent, but other than overfiring in most browsers it seems to be ok. This is a browser issue in general so will hopefully eventually get fixed so that the onChange on the slider only triggers when the slider is 'dropped' instead of each point along the way, as per the standard
@dpwrussell
Copy link
Collaborator Author

@pwalczysko The reason that it was doing that was an attempt to get around inconsistencies in how range sliders are treated by the various browsers, but I guess it ended up being worse than just letting the browsers do the wrong thing some of the time. It should now work normally on Safari.

According to the standard, the range slider is only supposed to fire the onChange event when the nob is 'dropped', when dragging it. The browsers are all over the map on this, but hopefully it'll get fixed down the line.

@pwalczysko
Copy link

@dpwrussell : Understood. So you mean that you fixed the behaviour on Safari just now ? Do not see any new commit.

@dpwrussell
Copy link
Collaborator Author

Damn airport wifi was playing up, I lost connection right after I posted that and couldn't push. Am UK side now and pushing.

@dpwrussell
Copy link
Collaborator Author

This branch is complete.

@pwalczysko
Copy link

The bug on safari #37 (comment) is fixed as tested today on Mac OS 10.10

@pwalczysko
Copy link

All the bugs and improvements reported here were fixed. The PR is not breaking anything in the mainline.

@dpwrussell
Copy link
Collaborator Author

@pwalczysko +1

@dpwrussell
Copy link
Collaborator Author

Unless there are any objections, I will merge this tomorrow and do a release.

@dpwrussell dpwrussell merged commit df5a344 into German-BioImaging:master Apr 19, 2016
@dpwrussell dpwrussell deleted the react branch April 19, 2016 14:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants