-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Refactor title and subtitle selection in drawMainTitle function to use scoped selector #7522
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
base: master
Are you sure you want to change the base?
Conversation
…e context-specific selection from the graph div (gd).
test/jasmine/tests/titles_test.js
Outdated
|
||
it('computes title automargins independently when multiple plots exist', function(done) { | ||
var gd1 = gd; | ||
var gd2 = createShadowGraphDiv(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createShadowGraphDiv()
is a specialized function for testing graphs in the shadow DOM; I'd rather not use it here.
Instead let's modify the createGraphDiv()
function to accept an optional divId
argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR is updated! I included a cleanup to destroy the new graph div after the test. Let me know if you'd prefer I update destroyGraphDiv
to handle other IDs instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See one comment regarding the test; otherwise looks great. Thanks very much for the fix @davibarbosa2 . 🎉
…ter and update tests to utilize the new functionality.
I'm seeing a CI failure 😢 the tests are passing on my machine. Any chance you could restart the build for me? |
@davibarbosa2 @emilykl Please see my comment #7513 (comment) on how to fix the CI issue as a workaround in the GitHub workflow. I see that your jasmine error report shows the line plotly.js/test/jasmine/tests/transition_test.js Lines 646 to 652 in e8434c8
|
This pull request refactors the
drawMainTitle
function to improve how main titles and subtitles are selected.Previously, the function used a global
d3.selectAll
to select the.gtitle
and.gtitle-subtitle
elements. This could lead to incorrect selections if multiple plots were present on the same page.The changes in this PR modify the selection to be context-specific by using
d3.select(gd).selectAll
, ensuring that the title and subtitle elements are selected only from within the current graph div (gd
). This prevents styling and layout conflicts between different plots.Closes #7521