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

Fixed error caused by getDarkMode in edit page #156

Merged
merged 9 commits into from
May 3, 2021
Merged

Fixed error caused by getDarkMode in edit page #156

merged 9 commits into from
May 3, 2021

Conversation

raashika03
Copy link
Contributor

Checklist

  • I indicated which issue (if any) is closed with this PR using closing keywords
  • I only changed lines related to my PR (no bulk reformating)
  • I indicated the source and check the license if I re-use code, or I did not re-use code
  • I made my best to solve only one issue in this PR, or explain why multi had to be solved at once.

Issue

fix #154

Details

this went unnoticed when I proposed those changes.

@raashika03
Copy link
Contributor Author

Thank you @bryan-brancotte for this Removal. I was also doubtful whether to have a single button for the whole website or let it be on all pages.
This's seems better now.

@raashika03
Copy link
Contributor Author

That bottom-border colour was needed to be changed, cause we changed class="page-header" to id="page-header" for the front page in #143, so those styling which this edit page was using get changed so added it separately.
@bryan-brancotte

Copy link
Member

@bryan-brancotte bryan-brancotte left a comment

Choose a reason for hiding this comment

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

You indeed found an issue, but the PRis partially fixing it. I recommend you start a first PR with the css fix which is sound (I'll merge it right away). And a second in which you will investigate why there is a js issue.

js/edit.js Outdated Show resolved Hide resolved
@raashika03 raashika03 mentioned this pull request Apr 29, 2021
4 tasks
@raashika03 raashika03 changed the title Fixing issues in propose pages [WIP]Fixing issues in propose pages Apr 29, 2021
@raashika03 raashika03 changed the title [WIP]Fixing issues in propose pages Fixing error caused by getDarkMode in edit page Apr 30, 2021
@raashika03 raashika03 changed the title Fixing error caused by getDarkMode in edit page Fixed error caused by getDarkMode in edit page Apr 30, 2021
js/edit.js Outdated Show resolved Hide resolved
@raashika03
Copy link
Contributor Author

Indentation change was required because of addition of if-else statement, so reformatting was needed. Please check @bryan-brancotte

Copy link
Member

@bryan-brancotte bryan-brancotte left a comment

Choose a reason for hiding this comment

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

Hi,

You indeed spotted the issue well, the solution is working, but is refactoring a too much.

Indentation

There was an incorrect indentation here and it is still present : when opening a new curly bracket indetation should go on level more.

Smaller test

Also, and even if your solution works, you could have done it with less refactoring :

  • Here you get the button
  • Here you add a listener
  • You could have simply tested right before if btn was null and then returning :
if (null == btn || undefinfed == btn)
    return;

@raashika03
Copy link
Contributor Author

@bryan-brancotte have a look.

@raashika03
Copy link
Contributor Author

If button isn't present then still we need to check for the current theme for application on edit page. We can't simply return no @bryan-brancotte

@bryan-brancotte
Copy link
Member

Capture d’écran de 2021-05-03 14-35-56

and after fixing the indentation, and removing a tab

function getDarkMode() {
    const btn = document.querySelector(".btn-toggle");
    const currentTheme = localStorage.getItem("theme");
    if (currentTheme == "dark") {
        document.body.classList.add("dark-mode");
    }
    if (null == btn || undefined == btn)
        return;
    btn.addEventListener("click", function () {
        document.body.classList.toggle("dark-mode");
    
        let theme = "light";
        if (document.body.classList.contains("dark-mode")) {
            theme = "dark";
        }
        localStorage.setItem("theme", theme);
    });
}

@raashika03
Copy link
Contributor Author

sorry for complicating things 😓 now good to merge

@bryan-brancotte
Copy link
Member

sorry for complicating things now good to merge

not complicating things, learning things

@bryan-brancotte bryan-brancotte merged commit f3cef05 into edamontology:main May 3, 2021
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.

error caused by getDarkMode in edit page
2 participants