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

build(deps): upgrade to blueprintjs@4 #1160

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aryanshridhar
Copy link
Collaborator

@aryanshridhar aryanshridhar commented Aug 11, 2022

Upgrades blueprintjs to v4 (4.8.0)
This also upgrades the following packages with it -

  • @blueprintjs/popover2@1.5.1
  • @blueprintjs/select@4.5.3

to make sure that the codebase relies on only v4 version of blueprintjs/core -

❯ yarn why @blueprintjs/core
yarn why v1.22.19
warning ../../package.json: No license field
[1/4] 🤔  Why do we have the module "@blueprintjs/core"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "@blueprintjs/core@4.8.0"
info Has been hoisted to "@blueprintjs/core"
info Reasons this module exists
   - Specified in "dependencies"
   - Hoisted from "@blueprintjs#popover2#@blueprintjs#core"
   - Hoisted from "@blueprintjs#select#@blueprintjs#core"
info Disk size without dependencies: "9.64MB"
info Disk size with unique dependencies: "61.88MB"
info Disk size with transitive dependencies: "69.61MB"
info Number of shared dependencies: 34
✨  Done in 0.75s.

Fixes #1045

@aryanshridhar
Copy link
Collaborator Author

aryanshridhar commented Aug 11, 2022

Quite funny to see this test fail here.
When logged out the html it clearly contains the .disabled-menu-tooltip class and surprisingly enzyme is not able to find it.
Here's the logged html -

    <span class="disabled-menu-tooltip bp4-popover2-target"><li class="" role="none"><a role="menuitem" tabindex="-1" label="Not downloaded" icon="cloud" class="bp4-menu-item bp4-active bp4-disabled bp4-selected"><span class="bp4-menu-item-icon"><span icon="cloud" aria-hidden="true" tabindex="-1" class="bp4-icon bp4-icon-cloud"><svg data-icon="cloud" width="16" height="16" viewBox="0 0 16 16" role="img"><path d="M12 6c-.03 0-.07 0-.1.01A5 5 0 002 7c0 .11.01.22.02.33A3.51 3.51 0 000 10.5C0 12.43 1.57 14 3.5 14H12c2.21 0 4-1.79 4-4s-1.79-4-4-4z" fill-rule="evenodd"></path></svg></span></span><div class="bp4-fill bp4-text-overflow-ellipsis">1.0.0</div><span class="bp4-menu-item-label">Not downloaded</span></a></li></span>

Not sure what's going wrong here 🤔

@georgexu99
Copy link
Contributor

Hey! I took a look at the failing test- in the interest of moving the PR forward, we can write the test using a snapshot instead as it seems that the wrapper is pulling some funnys ;)

We should rewrite the next test for the same reason- the wrapper may be giving us a false positive.

Here's a gist describing what I'm thinking. This would be similar to how we test menu items in the general appearance settings

@dsanders11
Copy link
Member

@georgexu99, that seems reasonable. Could you open a PR for just that change? This one is pretty big and hairy to review, so would be easier to land that generic test refactor separately.

@aryanshridhar
Copy link
Collaborator Author

Actually, I already have these changes in my local branch - https://github.com/aryanshridhar/fiddle/tree/Add-VersionSelectSnapshots
We can pick up from there if that seems fine :)

@dsanders11
Copy link
Member

@aryanshridhar, can we rebase this PR and get it clean again? I can further look into the test failure after that. 🙂

@aryanshridhar
Copy link
Collaborator Author

Rebased @dsanders11.
However, as this pr got really old (lot of merge conflicts); Manually testing the application now to check if everything works as desired.

@erickzhao erickzhao marked this pull request as draft November 7, 2023 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to BlueprintJS 4
4 participants