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

add basic code to generate the login button #2

Merged
merged 15 commits into from
Jan 26, 2024
Merged

Conversation

gumaerc
Copy link
Collaborator

@gumaerc gumaerc commented Jan 25, 2024

What are the relevant tickets?

Closes #1

Description (What does it do?)

This is the first PR for this library. First of all, it adds basic boilerplate linting / formatting / github actions CI config. The main purpose of this PR is to get started setting up the primary purpose of the library, which is to generate a login button pointing at an instance of mit-open. The library exports a function called initLoginButton, which generates a login button pointing at a provided mit-open instance and a simple text element showing who is logged in if you already are logged in.

Screenshots (if appropriate):

image
image

How can this be tested?

  • In order to test this, you will need a running instance of mit-open (https://github.com/mitodl/mit-open) on the cg/login-button-cors-settings branch. Follow the instructions in the mit-open readme for initial setup, check out the branch and spin it up. You will need an account in your local instance for this to work.
  • You will also need ocw-hugo-themes (https://github.com/mitodl/ocw-hugo-themes) set up locally, running a course using yarn start course. Follow the readme there for basic setup instructions and make sure you can start up a course. After you have that working, check out the cg/add-mit-open-login-button branch. Assuming you have cloned this repo, run yarn add /path/to/mit-open-login-button to add your local copy of the code for this to your locally running ocw-hugo-themes. This is necessary because the package is not yet published to NPM.
  • Once you have all this running, make sure you are logged out of your local instance of mit-open
  • Back in your locally running OCW course site, you should see a Login button in the upper right. Click this button and log in to your locally running mit-open
  • After you have logged in, you will be sent to http://localhost:8063
  • Go back to your locally running OCW site at http://localhost:3000 and confirm that you see Logged in as: username with your username in place of username
  • In another tab, log out of your locally running mit-open
  • Verify that the login button reappears

@gumaerc gumaerc added the Needs Review An open Pull Request that is ready for review label Jan 25, 2024
Copy link

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

Took a quick look at code, now going to test it out. A few small comments about setup stuff.


Suggestion: Currently it's not really clear what version of yarn is being used. The github action uses yarn 1.22.

My suggestion would be to use yarn 4:

  • add a nvmrc for Node 20
  • after activating, corepack enable
  • run yarn init -2
    • Check the .yarn/ folder. yarn 4 has some nice improvements IMO. In particular, we no longer need to store the yarn binary. Also, we get node_modules installation by default rather than their other form of package installation.
    • The one downside to yarn 4 that I am aware of is that it is a little awkward in github actions. For you moment, I believe you need to do this. Note the SKIP_YARN_COREPACK_CHECK and corepack enable. See also Corepack Support actions/setup-node#531 (comment)

index.js Outdated
) {
const container = document.getElementById(containerId)
const parsedBaseUrl = new URL(baseUrl)
const reconstructedBaseUrl = `${parsedBaseUrl.protocol}//${parsedBaseUrl.hostname}${parsedBaseUrl.port !== "" ? `:${parsedBaseUrl.port}` : ""}`

Choose a reason for hiding this comment

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

Is this the same as parsedBaseUrl.origin ?

package.json Outdated
"scripts": {
"lint": "eslint './**/*.{js,jsx,ts,tsx}'",
"fmt": "yarn fmt:check --write",
"fmt:check": "LOG_LEVEL= prettier-eslint --list-different './**/*.{js,jsx,ts,tsx,scss}'"

Choose a reason for hiding this comment

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

Strong Suggestion: Don't use prettier-eslint. Just use eslint, and drop a .pre-commit-config.yaml file in the repo.

Copy link

@ChristopherChudzicki ChristopherChudzicki left a comment

Choose a reason for hiding this comment

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

👍 👍 Cool.

Up to you if/when you want to make those suggested changes.

@ChristopherChudzicki ChristopherChudzicki added Waiting on Author and removed Needs Review An open Pull Request that is ready for review labels Jan 26, 2024
package.json Outdated
"eslint-plugin-react": "^7.33.2",
"eslint-plugin-react-hooks": "^4.6.0",
"typescript": "^5.3.3",
"typescript-eslint": "^0.0.1-alpha.0"

Choose a reason for hiding this comment

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

you don't want this.

(Do you want any typescript right now?)

@gumaerc gumaerc merged commit 307d9e2 into main Jan 26, 2024
2 checks passed
@gumaerc gumaerc deleted the cg/add-init-login-button branch January 26, 2024 21: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.

generate a button for logging into MIT Open
2 participants