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

[Draft] don't reload searchkit on edit #27098

Closed
wants to merge 1 commit into from

Conversation

MegaphoneJon
Copy link
Contributor

Overview

When an inline edit field in SearchKit is modified, the entire query is reloaded. This aims to make the UI snappier.

To test, you can create any SK query with in-line edit fields - though the difference will be far more pronounced with more columns/results. You can see in the Network tab that the call to SearchDisplay.run doesn't occur.

Before

Modifying a field results in several seconds' wait (if you have enough columns).

After

Super-fast.

Comments

This successfully updates the database, and if you go to edit again, your value will be present. But the new value doesn't display on screen post-save, even though my AngularJS plugin says the scope is updated.

I've been trying to learn AngularJS but I've spent more hours than I'd like to say trying to finish this - any pointers on this last part would be helpful.

@civibot
Copy link

civibot bot commented Aug 19, 2023

Thank you for contributing to CiviCRM! ❤️ We will need to test and review the PR. 👷

Introduction for new contributors
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers

@civibot civibot bot added the master label Aug 19, 2023
@colemanw
Copy link
Member

Sure @MegaphoneJon, I think the problem here is not so much the Angular but the fact that rendering of display values is done server-side so we have to imitate that using js to update not just the raw value but also the display value.
I've got something mostly working locally that I'll tweak a bit more. Care to trade for a review of #27054 (which is a prerequisite for the performance improvements we've been discussing)

@colemanw
Copy link
Member

Yea it was tricky. Try out #27105

@colemanw colemanw closed this Aug 20, 2023
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.

2 participants