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

Move update of graph theme to Graph.__call__ #174

Merged
merged 16 commits into from
Nov 15, 2023
Merged

Conversation

antonymilne
Copy link
Contributor

@antonymilne antonymilne commented Nov 14, 2023

Description

As prototyped in #166, a bit of further refactoring with no change in functionality.

Before: the code to update graph theme lived in _callback_mapping_utils.py.
Now: it lives in Graph, plus a couple of other improvements.

Why is this (probably) better?

  • callbacks no longer need to all carry around ctd_theme
  • callbacks are not directly tied to Graph implementation
  • the duplicated logic between the update_graph_theme callback and the update_layout call is centralised to a single Graph._update_theme method
  • the update_theme callback is generic so in theory works for components other than Graph so long as they define a _update_theme method (probably not very useful in practice)

Possible drawbacks:

  • Graph.__call__ is coupled to the Dash callback context. This was actually aways my original intention, but it was never made explicit before, and the current actions implementation works somewhat differently. To be seen whether it causes problems

Future work:

  • (small/easy) see if we can get rid of the _update_theme call in Graph.__call__ altogether by making update_theme a clientside callback. This would be even nicer, but the current solution is perfectly performant so it's not a big problem
  • (bigger/difficult) consider whether more functionality should move from callbacks into __call__

Checklist

  • I have not referenced individuals, products or companies in any commits, directly or indirectly
  • I have not added data or restricted code in any commits, directly or indirectly
  • I have updated the docstring of any public function/class/model changed
  • I have added tests to cover my changes (if applicable)

Types of changes

  • Docs/refactoring (non-breaking change which improves codebase)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

@antonymilne antonymilne changed the base branch from main to tidy/update-theme November 14, 2023 16:15
Base automatically changed from tidy/update-theme to main November 15, 2023 08:37
@antonymilne antonymilne marked this pull request as ready for review November 15, 2023 10:31
@antonymilne antonymilne changed the title Tidy/update graph theme Move update of graph theme to Graph.__call__ Nov 15, 2023
@petar-qb petar-qb self-requested a review November 15, 2023 10:35
Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

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

Great tidying up! 🚀

@antonymilne antonymilne enabled auto-merge (squash) November 15, 2023 15:54
@antonymilne antonymilne merged commit 4160de8 into main Nov 15, 2023
25 checks passed
@antonymilne antonymilne deleted the tidy/update-graph-theme branch November 15, 2023 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants