The Wayback Machine - https://web.archive.org/web/20210815112348/https://github.com/scikit-learn/scikit-learn/pull/19417
Skip to content

/ scikit-learn

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

Jump to bottom

FIX Gives a unique id to html visualization #19417

Merged
merged 4 commits into from Feb 15, 2021
Merged

FIX Gives a unique id to html visualization #19417

merged 4 commits into from Feb 15, 2021

Conversation

@thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Feb 9, 2021

Reference Issues/PRs

The CSS is now targeted to a specific container by ID, which helps isolated the CSS from other elements. Using an ID increases the specificity of the selector which gives the style in scikit-learn higher precedence.

What does this implement/fix? Explain your changes.

This resolves the issue here: INRIA/scikit-learn-mooc#146

Screen Shot 2021-02-09 at 1 56 08 PM

CC @lesteve
@thomasjpfan
FIX Gives a unique id to html visualization
c9792fd
@thomasjpfan
DOC Adds whats new
61b1e61
@thomasjpfan thomasjpfan added this to the 0.24.2 milestone Feb 9, 2021
@thomasjpfan
Copy link
Member Author

@thomasjpfan thomasjpfan commented Feb 9, 2021

Milestone this for 0.24.2 for now, open to have it moved to 1.0.
@thomasjpfan thomasjpfan marked this pull request as draft Feb 9, 2021
thomasjpfan added 2 commits Feb 9, 2021
@thomasjpfan
ENH Gives top container an id only
a8af4e2
@thomasjpfan
ENH Adds class name for backward compatiblity
bf4469e
@thomasjpfan thomasjpfan marked this pull request as ready for review Feb 9, 2021
@glemaitre
glemaitre approved these changes Feb 11, 2021
View changes
Copy link
Contributor

@glemaitre glemaitre left a comment

Happy with this change. @lesteve do you want to have a look at it
@lesteve
Copy link
Member

@lesteve lesteve commented Feb 11, 2021

This looks good to me (full disclosure: I didn't know you can have nested style inside a div).

Just to be sure, I'll double check this fixes the problem in the MOOC and merge if this is the case.
@lesteve
Copy link
Member

@lesteve lesteve commented Feb 11, 2021

I doubled-check and indeed this fixes it on the MOOC side, thanks a lot!

About including this in the 0.24.2 release this would be more convenient for the MOOC. The change seems innocuous enough but at the same time it is not a horrible bug that absolutely needs to be fixed in 0.24.2.

Since I am not involved in the day-to-day scikit-learn maintenance I am probably not the best person to decide so I'll let this one open for now ...
@lesteve
Copy link
Member

@lesteve lesteve commented Feb 15, 2021

OK I am going to merge this one!

Nobody complained and I have heard that 0.24.2 is not going to be a super quick bugfix-only release.
@lesteve lesteve merged commit b251f3f into scikit-learn:main Feb 15, 2021
25 checks passed
25 checks passed
@github-actions
triage
Details
@github-actions
Check build trigger
Details
@github-actions
Build wheel for cp${{ matrix.python }}-${{ matrix.platform_id }}-${{ matrix.manylinux_image }}
Details
@github-actions
Source distribution
Details
@github-actions
Upload to Anaconda
Details
@lgtm-com
LGTM analysis: C/C++ No code changes detected
Details
@lgtm-com
LGTM analysis: JavaScript No code changes detected
Details
@lgtm-com
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
@circleci-artifacts-redirector
ci/circleci: doc artifact Link to 0/doc/_changed.html
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
@azure-pipelines
scikit-learn.scikit-learn Build #20210209.52 succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Get Git Commit) Get Git Commit succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linting) Linting succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux py36_conda_openblas) Linux py36_conda_openblas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux py36_ubuntu_atlas) Linux py36_ubuntu_atlas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux pylatest_pip_openblas_pandas) Linux pylatest_pip_openblas_pandas succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux32 py36_ubuntu_atlas_32bit) Linux32 py36_ubuntu_atlas_32bit succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Linux_Runs pylatest_conda_mkl) Linux_Runs pylatest_conda_mkl succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py36_pip_openblas_32bit) Windows py36_pip_openblas_32bit succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (Windows py37_conda_mkl) Windows py37_conda_mkl succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (macOS pylatest_conda_forge_mkl) macOS pylatest_conda_forge_mkl succeeded
Details
@azure-pipelines
scikit-learn.scikit-learn (macOS pylatest_conda_mkl_no_openmp) macOS pylatest_conda_mkl_no_openmp succeeded
Details
@lesteve lesteve mentioned this pull request Feb 16, 2021
Closed
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
Merged
10 of 12 tasks
glemaitre added a commit to glemaitre/scikit-learn that referenced this pull request Apr 22, 2021
@thomasjpfan @glemaitre
FIX Gives a unique id to html visualization (scikit-learn#19417)
3986907
glemaitre added a commit that referenced this pull request Apr 28, 2021
@thomasjpfan @glemaitre
FIX Gives a unique id to html visualization (#19417)
26167b9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre

Assignees
No one assigned
Labels
module:utils To backport
Projects
None yet
Milestone
0.24.2
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@thomasjpfan @lesteve @glemaitre @cmarmo