Add dataTable plugin - Githubissues

Tim-Siu commented 6 months ago

What is the purpose of this pull request?

[ ] Documentation update
[ ] Bug fix
[x] Feature addition or enhancement
[ ] Code maintenance
[ ] DevOps
[ ] Improve developer experience
[ ] Others, please explain:

Overview of changes:

Contribute to #896

Added a dataTable plugin. When we put a table into <m-table sortable> tag, it will become a Sortable Table. When we put a table into <m-table searable> tag, it will become a Searchable Table. Both functions can be used together.

DataTable is used.

Sample usage:

<m-table sortable searchable>
| Name | Age | Country |
|------|-----|---------|
| John | 28  | USA     |
| Emily| 32  | Canada  |
| Michael | 41 | UK    |
| Sophia | 25 | Germany|
| David | 37  | Australia |
</m-table>

Result:

https://github.com/MarkBind/markbind/assets/61866948/4d5e3b58-a723-4e94-b4d2-441c95e3984d

Anything you'd like to highlight/discuss: This has the potential of breaking the rendering pipeline. I am still conducting tests.

Testing instructions: See overview.

Proposed commit message: (wrap lines at 72 characters)

Add dataTable plugin

Checklist: :ballot_box_with_check:

[x] Updated the documentation for feature additions and enhancements
[x] Added tests for bug fixes or features
[x] Linked all related issues
[x] No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

[ ] Major (when you make incompatible API changes)
[x] Minor (when you add functionality in a backward compatible manner)
[ ] Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

To be included in the release note for any feature that is made obsolete/breaking

Previously tables do not support searching and sorting. Authors can now enable these features by adding dataTable as a site plugin and add the respective syntax around the table.

Give a brief explanation note about:

what was the old feature that was made obsolete

any replacement feature (if any), and

how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 96.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 51.05%. Comparing base (cb59668) to head (e254c05).

Files	Patch %	Lines
packages/core/src/plugins/dataTable.ts	96.66%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #2446 +/- ## ========================================== + Coverage 50.80% 51.05% +0.25% ========================================== Files 125 126 +1 Lines 5437 5467 +30 Branches 1170 1174 +4 ========================================== + Hits 2762 2791 +29 - Misses 2380 2381 +1 Partials 295 295 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

yiwen101 commented 6 months ago

Hi @Tim-Siu thank you for the amazing work; After settling other issues and reviews, could you also help update the user doc regrading this?

damithc commented 6 months ago

Nice effort @Tim-Siu Some things to think about:

sortable sounds like an attribute, so something like <m-table sortable> seems more intuitive? Allows to add more attributes later too.
For number columns, sorting should be by value (i.e., 2 is smaller than 15), not lexicographic. Is it handled automatically? Or is there a way for the user to specify? e.g., <m-table sortable sorting="text, number, text">

itsyme commented 6 months ago

Great addition to tables @Tim-Siu!

I just have two small nits from a UI perspective.

Perhaps you may want to explore other options for the icon for the unordered state (currently arrow up and down) to something that depicts its function more clearly. In my opinion the current icon is resembles cursor: resize giving me an impression that I can resize/move the column.
I felt that the current design may be a little too distracting. Perhaps something like decreasing the opacity and upping it on hover could declutter the column headers a little. You can take some inspiration from MUI Tables

Great work so far!

kaixin-hc commented 6 months ago

looks p good to me! - but wondering 2 things

possible to add tests?
if we're allowing sorting for text and numbers, is it possible / useful to also support sorting by date? if its simple we can do it here if its troublesome perhaps another PR

Tim-Siu commented 6 months ago

Great addition to tables @Tim-Siu!

I just have two small nits from a UI perspective.

Perhaps you may want to explore other options for the icon for the unordered state (currently arrow up and down) to something that depicts its function more clearly. In my opinion the current icon is resembles cursor: resize giving me an impression that I can resize/move the column.

I felt that the current design may be a little too distracting. Perhaps something like decreasing the opacity and upping it on hover could declutter the column headers a little. You can take some inspiration from MUI Tables

Great work so far!

Sure! I will explore different UI. I think there are some existing implementations on the internet for bootstrap tables and I will try to refer to them.

Tim-Siu commented 6 months ago

Hi @damithc ,

I updated the plugin and it is now able to support both sorting and filtering. This is all, however, powered by DataTables, which is a Javascript table library that is loaded from CDN directly. What do you think of the functioinality now? There are additional features like paging and scrolling from DataTable. Are these features needed?

Thanks for your advice.

damithc commented 6 months ago

I updated the plugin and it is now able to support both sorting and filtering. This is all, however, powered by DataTables, which is a Javascript table library that is loaded from CDN directly. What do you think of the functioinality now? There are additional features like paging and scrolling from DataTable. Are these features needed?

@Tim-Siu Good find. I'm no objections to using a third-party plugin, if the license is permissible. Another issue is the reliability of the CDN. What's the syntax for enabling filtering and sorting? What are the other features you think that might be relevant to our context?

damithc commented 6 months ago

Other concerns to check on:

How mature the plugin is
Is it currently active and likely to stay active for many years
The likelihood that the free version will be crippled in some ways in future, to push people to buy the commercial version

Tim-Siu commented 6 months ago

Other concerns to check on:

How mature the plugin is

Is it currently active and likely to stay active for many years

The likelihood that the free version will be crippled in some ways in future, to push people to buy the commercial version

Hi @damithc ,

Thank you for your input!

Regarding your concern:

The syntax for the plugin is that <m-table sortable searchable> to enable both features. We may also include only one feature.
The plugin appears to be quite popular and active. It is open source under MIT license. I have included the minimised js and css in our repo. It should be working fine as long as we are still using Bootstrap5. I guess we can still stick with these js and css files even if they commercialize it?
Some other features include paging. But I fell like it is a bit distracting as it wil add page info and agination control. It may be useful if we intend to have a really long table. But I think we already have panel, so it is not a necessary feature.

I do realise that the plugin is not working inside a panel or modal. It seems to suffer the same problem as the mermaid plugin #2052 . I am still investigating this issue.

damithc commented 6 months ago

I do realise that the plugin is not working inside a panel or modal. It seems to suffer the same problem as the mermaid plugin #2052 . I am still investigating this issue.

@Tim-Siu I see. That's a bummer, but if we figure out a way to fix it, we might be able to add both this and Mermaid to MarkBind, which would be REALLY great. Feel free to join forces with other team members (including mentors) to explore this, as this is likely hard to solve.

yucheng11122017 commented 6 months ago

Hi @Tim-Siu, can I check how to use this feature? I have added dataTable to the plugins and then used

<m-table sortable searchable>
| Name | Age | Country |
|------|-----|---------|
| John | 28  | USA     |
| Emily| 32  | Canada  |
| Michael | 41 | UK    |
| Sophia | 25 | Germany|
| David | 37  | Australia |
</m-table>

But it doesn't seem to work.

Please add some documentation so that we can test it easily.

Tim-Siu commented 6 months ago

Hi @Tim-Siu, can I check how to use this feature? I have added dataTable to the plugins and then used
<m-table sortable searchable>
| Name | Age | Country |
|------|-----|---------|
| John | 28  | USA     |
| Emily| 32  | Canada  |
| Michael | 41 | UK    |
| Sophia | 25 | Germany|
| David | 37  | Australia |
</m-table>
But it doesn't seem to work.

Please add some documentation so that we can test it easily.

Hi @yucheng11122017 ,

That is the correct usage.

It does work on my side. Can you paste a screenshot of the output on your side?

https://github.com/MarkBind/markbind/assets/61866948/1fdc9acd-5fb3-4e4b-82b4-a0360ecb0c57

yucheng11122017 commented 6 months ago

Hi @Tim-Siu could you get rid of the changes for the font files and emoji files and so on? Those shouldn't be changed unless you are doing an update on that. Thanks!

Tim-Siu commented 6 months ago

Hi @Tim-Siu could you get rid of the changes for the font files and emoji files and so on? Those shouldn't be changed unless you are doing an update on that. Thanks!

Hi @Tim-Siu could you get rid of the changes for the font files and emoji files and so on? Those shouldn't be changed unless you are doing an update on that. Thanks!

Hi @yucheng,

I think I did not make changes to the font files and emoji files. Those appear to be expected files generated when running npm run updatetest. Are there better practices I should adhere to when writing functional test?

Thanks in advance.

yucheng11122017 commented 6 months ago

Hi @Tim-Siu could you get rid of the changes for the font files and emoji files and so on? Those shouldn't be changed unless you are doing an update on that. Thanks!

Hi @Tim-Siu could you get rid of the changes for the font files and emoji files and so on? Those shouldn't be changed unless you are doing an update on that. Thanks!

Hi @yucheng,

I think I did not make changes to the font files and emoji files. Those appear to be expected files generated when running npm run updatetest. Are there better practices I should adhere to when writing functional test?

Thanks in advance.

Yes that happens sometimes... we usually just discard those files before pushing

yucheng11122017 commented 6 months ago

You can refer to this part of the documentation!

https://markbind.org/devdocs/devGuide/bootcamp/fixABug.html#updating-functional-tests

You should always check that the generated output is correct before committing any changes to the test sites.

Note that some binary files such as images (e.g. inline-output.png) or fonts (e.g. material-icons-outlined.woff) could show up as uncommitted changes due to the way they are generated. If you are not directly modifying those files in your PR, you should discard those changes and do not commit them.

Tim-Siu commented 6 months ago

You can refer to this part of the documentation!

https://markbind.org/devdocs/devGuide/bootcamp/fixABug.html#updating-functional-tests

You should always check that the generated output is correct before committing any changes to the test sites.

Note that some binary files such as images (e.g. inline-output.png) or fonts (e.g. material-icons-outlined.woff) could show up as uncommitted changes due to the way they are generated. If you are not directly modifying those files in your PR, you should discard those changes and do not commit them.

Sorry for the oversight. Thanks for the material!

yucheng11122017 commented 6 months ago

Hi @Tim-Siu sorry it seems to be working now.. There is weird scroll bar here though. Do you know what is causing this?

Tim-Siu commented 6 months ago

Hi @Tim-Siu sorry it seems to be working now.. There is weird scroll bar here though. Do you know what is causing this?

Hi @yucheng11122017 ,

I noticed this as well. I will look into it.

Tim-Siu commented 6 months ago

Other concerns to check on:

How mature the plugin is

Is it currently active and likely to stay active for many years

The likelihood that the free version will be crippled in some ways in future, to push people to buy the commercial version

Hi @damithc ,

Thank you for your input!

Regarding your concern:

The syntax for the plugin is that <m-table sortable searchable> to enable both features. We may also include only one feature.

The plugin appears to be quite popular and active. It is open source under MIT license. I have included the minimised js and css in our repo. It should be working fine as long as we are still using Bootstrap5. I guess we can still stick with these js and css files even if they commercialize it?

Some other features include paging. But I fell like it is a bit distracting as it wil add page info and agination control. It may be useful if we intend to have a really long table. But I think we already have panel, so it is not a necessary feature.

I do realise that the plugin is not working inside a panel or modal. It seems to suffer the same problem as the mermaid plugin #2052 . I am still investigating this issue.

Upon suggestions from @yucheng11122017 , I discovered that the inclusion of the m-table inside panel works if we specified preload.

yucheng11122017 commented 6 months ago

Upon suggestions from @yucheng11122017 , I discovered that the inclusion of the m-table inside panel works if we specified preload.

Yes this seems to be the issue. Because the HTML isn't rendered until the user opens it, the plugin can't process it. This should likely be the same issue with Mermaid (@yiwen101 and @LamJiuFong) and with the full site search as well as @jingting1412 and I were discussing.

@damithc said that he is okay with having this compromise - ie to integrate these features (full site search, tables and mermaids) but require panels to be preloaded by the user. This can be a workaround for now but we might need to think of a better way...

damithc commented 6 months ago

@damithc said that he is okay with having this compromise - ie to integrate these features (full site search, tables and mermaids) but require panels to be preloaded by the user. This can be a workaround for now but we might need to think of a better way...

@yucheng11122017 I'm OK to use this compromise for Tables and Mermaid (as the author can immediately spot the problem and apply the fix, and preload is needed only for some panels) but not for search (as it is harder for authors to spot the problem, and preload is needed for all panels).

Tim-Siu commented 6 months ago

Hi!

I seem to have made the plugin work in Vue components.

The original problem is that the DataTables initilization takes place when the page loads. However, when the table is inside a Vue component, the component's rendering process is not done at the time the initialization script runs. As a result, the DataTables initialization cannot find the table elements or properly attach the functionality to them.

The Solution is that I leveraged Vue's custom directives feature. Custom directives allow us to encapsulate DOM manipulations and execute them at specific stages of a component's lifecycle.

I defined a custom directive called v-datatable using Vue.directive(). Inside the directive, I specified an inserted hook, which is called when the directive is inserted into the DOM. In this hook, I initialized DataTables on the element using $(el).DataTable().
I updated the initScript to include the custom directive definition before the regular DataTables initialization code.
In the postRender function, I added the v-datatable directive to the table elements using .attr('v-datatable', ''). This ensures that the custom directive is applied to the tables within the Vue component.

When the Vue component is rendered, the custom directive's inserted hook is triggered for each table element with the v-datatable directive. This hook initializes DataTables on the table element, ensuring that the functionality is properly attached.

Tim-Siu commented 6 months ago

Hi @Tim-Siu could you get rid of the changes for the font files and emoji files and so on? Those shouldn't be changed unless you are doing an update on that. Thanks!

HI @yucheng11122017 ,

It seems like that the font and emoji related files are also present in other directories, e.g., test_siteand test_site_algoloa_plugin. It appears to me that they are necessary in order for the functional test to be sucessfully run.

Tim-Siu commented 6 months ago

Hi @Tim-Siu sorry it seems to be working now.. There is weird scroll bar here though. Do you know what is causing this?

Hi @yucheng11122017 ,

I am not quite sure how the scroll bar appears. It appears when I initialised with default template and using consecutive . But the scroll bar doesn't not apepar if I do the same thing starting from minimal template.

Since there should never be one, I added additional css to suppress it. It Hopefully this is the right fix.

kaixin-hc commented 6 months ago

I'll take a look at the rest of this later - but for the scrollbar fix, I think without isolating why it's happening, this CSS suppression feels a bit hacky to me. At very least maybe you should leave a comment above those lines of code to point to this issue so that if someone reads the code later they'll know when this surfaces - I can easily imagine someone removing what looks like superfluous CSS leading to a code regression

yucheng11122017 commented 6 months ago

Hi @Tim-Siu could you get rid of the changes for the font files and emoji files and so on? Those shouldn't be changed unless you are doing an update on that. Thanks!

HI @yucheng11122017 ,

It seems like that the font and emoji related files are also present in other directories, e.g., test_siteand test_site_algoloa_plugin. It appears to me that they are necessary in order for the functional test to be sucessfully run.

@Tim-Siu this shouldn't be true. You aren't making changes to these font and emoji files and hence there shouldn't be a change.

Tim-Siu commented 6 months ago

Hi @Tim-Siu could you get rid of the changes for the font files and emoji files and so on? Those shouldn't be changed unless you are doing an update on that. Thanks!

HI @yucheng11122017 ,

It seems like that the font and emoji related files are also present in other directories, e.g., test_siteand test_site_algoloa_plugin. It appears to me that they are necessary in order for the functional test to be sucessfully run.

@Tim-Siu this shouldn't be true. You aren't making changes to these font and emoji files and hence there shouldn't be a change.

Hi @yucheng11122017 ,

I think this has to do with how I am adding the functional test. From my observation of the existing convention in MarkBind, plugin related functional tests are placed in a standalone directory like test_site_algoloa_plugin. The font files and emoji files are indeed present in that directory. You may find them under paths like packages/cli/test/functional/test_site_algoloa_plugin/expected/markbind/css/fonts/KaTeX_AMS-Regular.ttf

yucheng11122017 commented 6 months ago

Hi @Tim-Siu could you get rid of the changes for the font files and emoji files and so on? Those shouldn't be changed unless you are doing an update on that. Thanks!

HI @yucheng11122017 ,

It seems like that the font and emoji related files are also present in other directories, e.g., test_siteand test_site_algoloa_plugin. It appears to me that they are necessary in order for the functional test to be sucessfully run.

@Tim-Siu this shouldn't be true. You aren't making changes to these font and emoji files and hence there shouldn't be a change.

Hi @yucheng11122017 ,

I think this has to do with how I am adding the functional test. From my observation of the existing convention in MarkBind, plugin related functional tests are placed in a standalone directory like test_site_algoloa_plugin. The font files and emoji files are indeed present in that directory. You may find them under paths like packages/cli/test/functional/test_site_algoloa_plugin/expected/markbind/css/fonts/KaTeX_AMS-Regular.ttf

Ah ok I see my bad. I realised that you created a new test site - not added on to the original test site.

yucheng11122017 commented 6 months ago

Hi @Tim-Siu please also add some documentation on how to use this plugin the user guide?

damithc commented 6 months ago

Folks, shall we try to wrap this up soon? As this is a new feature, I would like to use it in production before we wrap up this semester (so that we can fix any post-release issues quickly, if any). So, sooner we release, the more time we have to detect/fix such issues.

LamJiuFong commented 5 months ago

LGTM! Thanks for the work @Tim-Siu

damithc commented 5 months ago

Feature in action: https://nus-cs2103-ay2324s2.github.io/website/schedule/week4/project.html#2-review-some-peer-prs-fri-feb-9th-1600-counted-for-participation

MarkBind / markbind

Add dataTable plugin #2446

Codecov Report