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

fix: disabled download button for certain versions on darwin arm64 #1019

Merged
merged 26 commits into from
Apr 12, 2022

Conversation

Ayman161803
Copy link
Contributor

@Ayman161803 Ayman161803 commented Mar 19, 2022

Fixes #1017

I have disabled the download button for certain versions on darwin-arm64 as suggested by @dsanders11. I used the semver package to compare versions and added necessary stylings to commands.less. I have tested the changes made with results as given below.
Screenshot from 2022-03-19 12-15-26

The sample pic above is when condition process.platform==="linux" && process.arch==="x64" && semver.lt(item.version,"15.0.0") is applied to disable buttons

@Ayman161803 Ayman161803 changed the title fix: Disabled download button for certain versions on mac arm64 fix: Disabled download button for certain versions on darwin arm64 Mar 19, 2022
@dsanders11 dsanders11 changed the title fix: Disabled download button for certain versions on darwin arm64 fix: disabled download button for certain versions on darwin arm64 Mar 19, 2022
@dsanders11
Copy link
Member

Thanks for taking this up @Ayman161803. I was actually referring to the download buttons under preferences when I made that issue, but you're right, the ones in the version select also have the same problem. 😄

Fiddle Prefs -> Electron

Unfortunately I think this needs to be changed a bit. The changes to VersionSelect will break other functionality, such as in src/renderer/components/dialog-bisect.tsx, which makes use of the itemDisabled prop. There's multiple possible reasons a version in VersionSelect might be disabled, so any change needs to keep that in mind. Maybe that change for VersionSelect should be entirely contained within renderItem.

Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

Let's also cover the download buttons in the Electron section in preferences.

@Ayman161803
Copy link
Contributor Author

Ayman161803 commented Mar 19, 2022

Hey @dsanders11! I have made recommended changes to this PR i.e. shifted disable logic to renderItem and disabled appropriate download buttons on preference page as well. Please take the look at the same. Here is a sample pic showing the changed preference section.

Screenshot from 2022-03-19 15-33-59

@coveralls
Copy link

coveralls commented Mar 21, 2022

Coverage Status

Coverage increased (+0.03%) to 93.209% when pulling bc4961c on Ayman161803:patch-2 into f6ff9ea on electron:main.

src/less/components/commands.less Outdated Show resolved Hide resolved
src/renderer/components/settings-electron.tsx Outdated Show resolved Hide resolved
src/renderer/components/version-select.tsx Outdated Show resolved Hide resolved
@BlackHole1
Copy link
Member

Before adding unit tests, you also need to update the snapshots.
Run yarn run test -- -u for automatic updates

Co-authored-by: Black-Hole <158blackhole@gmail.com>
@Ayman161803
Copy link
Contributor Author

Ayman161803 commented Mar 22, 2022

Hey @BlackHole1! I have committed recommended changes. I am pretty new to writing unit tests. Please recommend changes, if any.

Also, I tried updating snapshots as well but it doesn't look like they are getting updated.

Screenshot from 2022-03-22 14-26-11

@BlackHole1
Copy link
Member

Component unit tests e.g.

@@ -0,0 +1,42 @@
diff --git a/tests/renderer/components/version-select-spec.tsx b/tests/renderer/components/version-select-spec.tsx
index 3a6bab6..16bdb50 100644
--- a/tests/renderer/components/version-select-spec.tsx
+++ b/tests/renderer/components/version-select-spec.tsx
@@ -14,12 +14,15 @@ import {
   renderItem,
   VersionSelect,
 } from '../../../src/renderer/components/version-select';
+import { disableDownload } from '../../../src/utils/disable-download';
 
 import { StateMock, VersionsMock } from '../../mocks/mocks';
 
 const { downloading, ready, unknown, unzipping } = VersionState;
 const { remote, local } = VersionSource;
 
+jest.mock('../../../src/utils/disable-download.ts');
+
 describe('VersionSelect component', () => {
   let store: StateMock;
 
@@ -85,6 +88,21 @@ describe('VersionSelect component', () => {
 
       expect(item).toBe(null);
     });
+
+    it('xxxxx', () => {
+      (disableDownload as any).mockReturnValueOnce(true);
+
+      const item = renderItem(mockVersion1, {
+        handleClick: () => ({}),
+        index: 0,
+        modifiers: { active: true, disabled: false, matchesPredicate: true },
+        query: '',
+      })!;
+
+      const ItemWrapper = shallow(item);
+
+      expect(ItemWrapper.find('.disabled-menu-tooltip')).toHaveLength(1);
+    });
   });
 
   describe('getItemLabel()', () => {

Copy link
Member

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

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

LGTM.

cc: @dsanders11

tests/renderer/components/settings-electron-spec.tsx Outdated Show resolved Hide resolved
tests/utils/disable-download-spec.ts Outdated Show resolved Hide resolved
src/utils/disable-download.ts Show resolved Hide resolved
src/utils/disable-download.ts Show resolved Hide resolved
@BlackHole1
Copy link
Member

BlackHole1 commented Mar 26, 2022

Done🚀. All unit test pass 🎉

@BlackHole1
Copy link
Member

Ping @dsanders11

Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

LGTM.

Sorry it took so long to circle back around on this @Ayman161803. Thanks for implementing it!

@BlackHole1 BlackHole1 merged commit be5d30b into electron:main Apr 12, 2022
@Ayman161803
Copy link
Contributor Author

Heyy! Thanks for having this merged.

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.

On Apple Silicon, Can't Download Pre-11.0 Versions
4 participants