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

Added Remember Me Feature #141

Merged
merged 4 commits into from
Oct 7, 2020
Merged

Added Remember Me Feature #141

merged 4 commits into from
Oct 7, 2020

Conversation

tektaxi
Copy link
Collaborator

@tektaxi tektaxi commented Oct 3, 2020

Added Remember Me Feature using localstorage
Closes #112

psvenk added 3 commits October 3, 2020 11:53
Normalize spacing and indentation, change "let" to "const" where
appropriate, use triple-equals, and change some names to be more
semantically accurate
Also make use of event listeners and make a few adjustments to make the
code more idiomatic in other areas.
Copy link
Member

@psvenk psvenk left a comment

Choose a reason for hiding this comment

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

@tektaxi Thank you for your contribution. I made a few changes:

  • Deleted "my first commit" (using a rebase; if you are interested, you
    can learn more about this technique at https://git-rebase.io/)
    because it is not necessary to include this in the commit log.

  • Normalized spacing and indentation: you should take a look at commit
    c8040c2 to see what spacing changes I made (you can also take a look
    at other code in the Aspine codebase, namely scrape.js, serve.js,
    and public/js/home.js, to see what the code style is like). As for
    indentation, you should install the EditorConfig plugin (or enable
    the EditorConfig functionality) for your text editor; if your text
    editor does not have such functionality, I would suggest using one of
    the editors listed on that webpage. My recommendations are Visual Studio
    Code, Emacs, and Vim (and my personal favorite of the three is Vim),
    though Notepad++ has served me well on Windows.

  • Made a few other code style changes: I changed let to const in
    some places; the difference between the two is that let allows a
    variable to be reassigned while const does not. It is generally seen
    as good style to use const whenever you do not need to reassign a
    variable. In addition, I changed one instance of == to === and
    renamed a few IDs and variable names to be more semantically accurate. I
    also added semicolons to every statement in your code to be consistent
    with the rest of the codebase (and because JavaScript's automatic
    semicolon insertion sometimes works counterintuitively).

  • Moved rememberMe and autofill functions from app.js to a new
    file, login.js, and used event listeners instead of onload and
    onclick. The file app.js is indeed the only JavaScript file used on
    the login page, but it is meant for setting up service workers for
    offline use (this functionality currently is not working) and is also
    called from the main Aspine page. So, I created a new file login.js
    with the login-page–specific code. This allowed me to use event
    listeners instead of making use of onload and onclick, which puts
    all of the logic together in the JavaScript file with just the markup in
    the HTML file.

  • Changed the <span> holding the text "Remember me?" to a <label> so
    that the user can click on the text to toggle the checkbox.

Otherwise, this looks good to me. I'll merge this (squashed into one
commit) once someone else takes a look.

@tektaxi
Copy link
Collaborator Author

tektaxi commented Oct 3, 2020

@psvenk Okay great. Thanks for the help! I'll get better at the syntax like let and const, semicolons, etc. Just not super used to it at the moment.

@psvenk psvenk requested review from kdk1616 and notrodes October 3, 2020 17:52
Copy link
Collaborator

@kdk1616 kdk1616 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@kdk1616 kdk1616 merged commit d822da0 into Aspine:master Oct 7, 2020
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.

"Remember me" using localStorage
3 participants