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

Scatterplot facet UI regression #3228

Closed
wetneb opened this issue Sep 28, 2020 · 14 comments · Fixed by #3338
Closed

Scatterplot facet UI regression #3228

wetneb opened this issue Sep 28, 2020 · 14 comments · Fixed by #3338
Labels
facets Behaviour or rendering of facets in a project Good First Issue Indicates issues suitable for newcomers to design or coding, providing a gentle introduction. Module: Frontend These issues involve working on HTML, CSS, and JavaScript code that affects the user interface. Type: Bug Issues related to software defects or unexpected behavior, which require resolution.
Milestone

Comments

@wetneb
Copy link
Sponsor Member

wetneb commented Sep 28, 2020

The UI of the scatterplot has got more ugly between 3.4 and the current development version.

To Reproduce

Steps to reproduce the behavior: create any scatterplot facet.

Current Results (master)

image

Expected Behavior (3.4)

image

Screenshots

Versions

  • Operating System: Linux
  • Browser Version: Firefox 68
  • JRE or JDK Version: 11
@wetneb wetneb added Type: Bug Issues related to software defects or unexpected behavior, which require resolution. Module: Frontend These issues involve working on HTML, CSS, and JavaScript code that affects the user interface. Good First Issue Indicates issues suitable for newcomers to design or coding, providing a gentle introduction. facets Behaviour or rendering of facets in a project labels Sep 28, 2020
@thadguidry
Copy link
Member

@tfmorris Likely from the Jquery update I suspect?

@wetneb wetneb added this to the 3.5 milestone Sep 28, 2020
@tfmorris
Copy link
Member

tfmorris commented Sep 28, 2020

I also noted that the rotate 45 degree buttons don't seem to work, perhaps due to incomplete jQuery Migrate support for the deprecated jquery.fn.load() call

migrateWarn | @ | project-bundle.js:11061
-- | -- | --
  | jQuery.fn.<computed> | @ | project-bundle.js:11574
  | load_image | @ | project-bundle.js:48576
  | load_images | @ | project-bundle.js:48565
  | (anonymous) | @ | project-bundle.js:48585
  | fire | @ | project-bundle.js:3234
  | fireWith | @ | project-bundle.js:3364
  | done | @ | project-bundle.js:9842
  | callback | @ | project-bundle.js:10313
  | XMLHttpRequest.send (async) |   |  
  | (anonymous) | @ | VM27946:1
  | send | @ | project-bundle.js:10256
  | ajax | @ | project-bundle.js:9740
  | jQuery.<computed> | @ | project-bundle.js:9892
  | getJSON | @ | project-bundle.js:9873
  | ScatterplotDialog._renderMatrix | @ | project-bundle.js:48473
  | ScatterplotDialog._createDialog | @ | project-bundle.js:48459
  | ScatterplotDialog | @ | project-bundle.js:48413
  | click | @ | project-bundle.js:43950
  | (anonymous) | @ | project-bundle.js:36142
  | dispatch

The style changes are likely due to either jQuery UI class name changes (e.g. ui-checkboxradio) causing overrides to no longer work, or some direct hacking on the jQuery UI style files which got lost in the jQuery UI upgrade.

The two styling problems are:

  • the boxes are too big, messing up the cropping for the sprites
  • the large selected (yellow)/not selected (orange) icons in the middle of the buttons where the labels need to be

@rachittiwari8562
Copy link
Contributor

rachittiwari8562 commented Nov 11, 2020

Can you please explain me the work of two span elements which are lying inside label in all the input fields like lin log etc.I am also attaching a screenshot here.
Screenshot from 2020-11-12 00-01-34

@tfmorris
Copy link
Member

Thanks for taking a look at this!

The second span contains the text label like "lin" for linear or "log" for logarithmic. The first span would, I'm guessing be the span which normally contains the icon, which we don't use (perhaps why it's get the class ui-icon-blank). If you look at the original problem report, it has a screenshot of how it is supposed to look. You can also run OpenRefine 3.4.1 to compare the HTML/CSS when it is working correctly.

@tfmorris
Copy link
Member

Update - that first icon span isn't in 3.4.1, it needs to be gotten rid of. You're definitely on the right track. The other thing that needs to be done is to resize the buttons so that they are back to their original smaller size. Good luck!

@rachittiwari8562
Copy link
Contributor

In the jquery-ui.js which is located at main/webapp/modules/core/externals/jquery-ui
In line no. 6519 there is used an jquery api to convert normal radio buttons to themeable buttons . There is a option named icon which is True which generates the radio button via span and if it false then the radio button disappears.I am attaching photo of the document.
Screenshot from 2020-11-14 01-25-53
photo of buttons after turning the icon off:
Screenshot from 2020-11-14 01-39-29
But I am not sure whether its the right way as it may affect other radio labels too which requires a icon in their button.

@tfmorris
Copy link
Member

That sounds promising. You obviously don't want to modify the source of jquery-ui, since that's a standard library, but I suspect you should be able to turn off the icon, just for the buttons in these two dialogs, using an appropriately specific CSS selector.

@rachittiwari8562
Copy link
Contributor

The bug is due to the jquery-ui widget function checkboxradio() which was adding icon to the input field.
Fix:
To override this a class "no-icon" is applied to individual input (radio) button on which we use checkbox("option","icon","false") to remove the icons.
Also added some CSS modification in the main/webapp/modules/core/styles/project/facets.less file .

Here are some sample photos:
Screenshot from 2020-11-17 18-58-26
Screenshot from 2020-11-17 18-58-35

@rachittiwari8562
Copy link
Contributor

Should i open a pull request??

@tfmorris
Copy link
Member

Looks great!

Should i open a pull request??

Yes, please. That will allow the team to review the changes and get them merged.

@rachittiwari8562
Copy link
Contributor

I am unable to resolve the rotate functionality of the facet and dialog .Actually i am having problem to understand the server side code .Is there any documentation available on it?.

@wetneb
Copy link
Sponsor Member Author

wetneb commented Nov 17, 2020

@rachittiwari8562 this is the documentation we have so far: https://docs.openrefine.org/manual/facets#scatterplot-facet
That being said I would not be surprised if there were bugs in this facet (even server side) independently of this regression, so if you notice something that does not look right in 3.4.1 about this, I would assume you are right and it is a genuine bug (or something worth improving), so feel free to open an issue about it.

@tfmorris
Copy link
Member

I've created #3371 for the remaining rotation button bug.

@rachittiwari8562
Copy link
Contributor

Sorry to bother you but earlier I had created the same issue #3344 .Please look at that issue and also provide me directions regarding the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
facets Behaviour or rendering of facets in a project Good First Issue Indicates issues suitable for newcomers to design or coding, providing a gentle introduction. Module: Frontend These issues involve working on HTML, CSS, and JavaScript code that affects the user interface. Type: Bug Issues related to software defects or unexpected behavior, which require resolution.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants