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

fix: use custom formatter when filtering rows #158

Merged
merged 1 commit into from Jun 10, 2022

Conversation

phot0n
Copy link
Contributor

@phot0n phot0n commented Jun 9, 2022

As datatable renders only some rows at a time and sets each
cell's html property based on that rendering, so when someone
filters things and only looks at some rows without scrolling the whole
grid and tries to filter things (when there is a custom cell formatter),
they won't be able to see the data as the filtering is done based on cell's
content not html (as it's not set at that point)

For example datatable renders date by default in the format: yyyy-mm-dd but if someone uses a custom formatter and changes it to say dd-mm-yyyy, they won't be able to filter/see the data properly

before:

Screen.Recording.2022-06-10.at.11.27.34.AM.mov

after:

Screen.Recording.2022-06-10.at.11.25.29.AM.mov

@phot0n phot0n force-pushed the fix-filter-custom-formatter branch 9 times, most recently from 8aec96d to 33332b8 Compare June 10, 2022 05:59
@phot0n phot0n marked this pull request as ready for review June 10, 2022 06:01
@@ -527,7 +527,7 @@ export default class CellManager {
const done = editor.setValue(value, rowIndex, col);

// update cell immediately
this.updateCell(colIndex, rowIndex, value);
this.updateCell(colIndex, rowIndex, value, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this for the case when someone edits a cell, as during editing we directly update the cell's content property - this will make sure to refresh the cell's html as well

const row = this.datamanager.getRow(cell.rowIndex);
const data = this.datamanager.getData(cell.rowIndex);
contentHTML = customFormatter(cell.content, row, cell.column, data);
if (!cell.html || refreshHtml) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this so that we don't keep on re-formatting cells which are already formatted/have html property set

As datatable renders only some rows at a time and sets each
cell's html property based on that rendering, so when someone
filters things and only looks at some rows without scrolling the whole
grid and tries to filter things (when there is a custom cell formatter),
they won't be able to see the data as the filtering is done based on cell's
content not html (as it's not set at that point)
@phot0n phot0n force-pushed the fix-filter-custom-formatter branch from 33332b8 to 430d7cf Compare June 10, 2022 06:21
@shariquerik shariquerik merged commit eda1540 into frappe:master Jun 10, 2022
@github-actions
Copy link

🎉 This PR is included in version 1.16.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@phot0n phot0n deleted the fix-filter-custom-formatter branch December 24, 2022 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants