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: add option for image size #221

Merged
merged 11 commits into from
Jan 20, 2021
Merged

Conversation

yuanLeeMidori
Copy link
Contributor

Hi, I add the feature to check different image sizes with keyboard shortcuts. This PR is for solving the issue #91.

While the user switch to the image tab,
z l change the search result to only large size;
z e change the search result to only medium size; (z m has been taken by filtering by past month)
z i change the search result to only icon size.

Take your time to review my code and please let me know if there is anything I can improve. Thank you!

src/main.js Outdated Show resolved Hide resolved
assets/chrome_webstore_description.txt Outdated Show resolved Hide resolved
src/options_page.html Outdated Show resolved Hide resolved
src/main.js Show resolved Hide resolved
src/search_engines.js Show resolved Hide resolved
// Expend dropdown and select the size again.
switch (size) {
case 'l':
if (dropDownWithSize.innerHTML == 'Large') {
Copy link
Owner

Choose a reason for hiding this comment

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

Will checking 'Large' works in other languages?

if (dropDownWithSize.innerHTML == 'Large') {
break;
} else {
dropDownWithSize.click();
Copy link
Owner

Choose a reason for hiding this comment

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

Here and below you are calling dropDownWithSize.click() but dropDownWithSize may be undefined?

@@ -444,6 +444,79 @@ class GoogleSearch {
}
return false;
}

changeImageSize(size) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think it's better to use as input to the function something more descriptive than 'i', 'l', 'e'. Javascript doesn't have enums but you can simulate them: https://stackoverflow.com/q/287903/1014208

// Expend dropdown and select the size again.
switch (size) {
case 'l':
if (dropDownWithSize.innerHTML == 'Large') {
Copy link
Owner

Choose a reason for hiding this comment

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

Here and below you are calling dropDownWithSize.innerHTML but dropDownWithSize may be undefined?

'[class="MfLWbb"][aria-label="Icon"]');
const dropDownWithSize = document.querySelector(
'[class="xFo9P r9PaP Fmo8N"][jsname="wLFV5d"]');
if (large != null && medium != null && icon != null) {
Copy link
Owner

Choose a reason for hiding this comment

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

It seems possible to simplify the two switch statements. Not sure about the best way to do it, but maybe something like:

switch(size) {
  case LARGE:
    if (dropDownWithSize != null) {
      if (dropDownWithSize.innerHTML == 'Large') {
        return;
      }
      if (large == null) {
        dropDownWithSize.click();
        // large = ...
      }
    }
    if (large != null) {
      large.click();
    }
  // ...
}

Now basically some of the code in the case can be extracted out to a function, say something like:

const setImageSize = (dropDownWithSize, getButton) => {
  let button = getButton();
  if (dropDownWithSize != null && button == null) {
    dropDownWithSize.click();
    button = getButton();
  }
  if (button != null) {
    button.click();
  }
};

// ...


switch(size) {
  case LARGE:
    if (dropDownWithSize != null && dropDownWithSize.innerHTML == 'Large') {
      return;
    }
    setImageSize(dropDownWithSize, () => { 
      // return document...;
    });
    break;
  // ...
}

@infokiller
Copy link
Owner

@yuanLeeMidori no pressure, but just making sure you know I'm waiting for changes I requested in earlier comments

@yuanLeeMidori
Copy link
Contributor Author

@infokiller Hi, thank you for the reminder. Sorry for keeping you wait. If you want, you can close this PR, and I'll send a new one with that meets these requirements once I get time to work on it. Thank you!

@infokiller
Copy link
Owner

Don't worry, there's no pressure. I just wanted to make sure you're not waiting for me :)

@yuanLeeMidori
Copy link
Contributor Author

@infokiller Hi! Sorry for keeping you wait. I've made some changes based on your requests. Please let me know if there is anything I can improve. Thank you so much.

@infokiller
Copy link
Owner

@yuanLeeMidori thanks a lot for these changes and your contribution! from a quick look it looks good, but I'll want to test it a bit on my computer. I'm a bit busy in the next few days, so I'll probably get to it in a week or so.

@yuanLeeMidori
Copy link
Contributor Author

@infokiller No worries! Take your time. Thank you for giving me the chance and help me to improve my code.

@infokiller infokiller merged commit 3f32f33 into infokiller:master Jan 20, 2021
@infokiller
Copy link
Owner

Thanks a lot @yuanLeeMidori it looks great! I just merged it

@yuanLeeMidori
Copy link
Contributor Author

@infokiller Thank you for the waiting and reviewing, I'm very grateful. Thank you for giving me the chance :)

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.

2 participants