search

jupyter-widgets / ipydatagrid

Fast Datagrid widget for the Jupyter Notebook and JupyterLab
BSD 3-Clause "New" or "Revised" License
579 stars 51 forks source link

Fix for #284 not working in Safari (mac/ios) #351

Closed mangecoeur closed 2 years ago

mangecoeur commented 2 years ago

Issue number of the reported bug or feature request: #284

Describe your changes Replaced the lookbehind regex in vegaexpr.ts with a normal regex group. Since the resulting matches are anyway re-processed with another regex, the extra capture group in the match shouldn't affect the final result.

Testing performed Ran python and js tests, confirmed working in jupyter lab in firefox and safari.

mangecoeur commented 2 years ago

I'm getting some visual regressions, it seems conditional formatting is not working as expected. Can someone point me how to run the visual regression tests locally? (I ran the JS tests but it doesn't seem to have run the visual tests)

ibdafna commented 2 years ago

Hello 👋 @mangecoeur and many thanks for submitting this PR! 🧡

This regex is quite tricky, and we should probably think if there's a way to completely get rid of it as it's too fiddly to be relied upon for conditional formatting. It looks like the conditional formatting fails specifically for compound objects, i.e. lists of lists.

To run the visual regression tests locally, you can cd into ui-tests-xxx folder for either ipywidgets 7 or 8 and run yarn install. You might need to install some system-level playwright components. If that's the case, you will see a prompt on the screen telling you what to do.

Once everything is installed, you should open two terminal windows - both in the ui-tests folder and in one window you should run the test server yarn start, and the test suite in the other: yarn test. Let us know how it goes! Thanks again for contributing 🙏

ibdafna commented 2 years ago

Fixing the DCO test should be easy:

For future commits, you can use git commit -s -m "some commit message". The -s flag signs the commit.

I'll try restarting the visual regression test for ipywidgets 7 - it's somewhat inconclusive there.

mangecoeur commented 2 years ago

@ibdafna thanks, I managed to make a complete mess of my repo trying to fix the DCO. I will try to sort that out and re-open the pull request

ibdafna commented 2 years ago

Oh man 😟 I'm sorry to hear that. Let me know if you need help - happy to jump on a call, too.