Skip to content

[dashboard] Fix Dashboard lint errors for Login.tsx #16158

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

Merged

Conversation

Palanikannan1437
Copy link
Contributor

@Palanikannan1437 Palanikannan1437 commented Feb 1, 2023

Related Issue(s)

Description

  • Fixes the lint issues in Login.tsx page
    • Fixes the absence of alt text for images in the welcome login page
    • Fixed the missing dependency of hostFromContext while fixing the multiple render bug!
  • Additional Improvement
    • The urlHash i.e. domain/#https://github.com/P...... part is calculated thrice in each page load to find the hostProvider, hence I've fixed it to just be calculated once by maintaining appropriate states while fixing the missing dependency issue!
Before (the hash calc function runs thrice) After (the function runs once)
before after

How to test

  • Go to ./components/dashboard
  • Run the command yarn run start and set appropriate environment variables GP_DEV_HOST & GP_DEV_COOKIE
  • Now you can see there's no lint errors regarding Login.tsx
Before After
image image

Release Notes

Fixed lint warnings and multiple re-render bug in Login page 

Build Options:

  • /werft with-github-actions
    Experimental feature to run the build with GitHub Actions (and not in Werft).
  • leeway-no-cache
    leeway-target=components:all
  • /werft no-test
    Run Leeway with --dont-test

Preview Environment Options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@Palanikannan1437 Palanikannan1437 requested a review from a team February 1, 2023 14:30
@roboquat
Copy link
Contributor

roboquat commented Feb 1, 2023

@Palanikannan1437: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@werft-gitpod-dev-com
Copy link

annotations in the pull request changed, but user is not allowed to start a job

@gtsiolis
Copy link
Contributor

gtsiolis commented Feb 1, 2023

/werft run

👍 started the job as gitpod-build-dashboard-lint-fix-loginpage-fork.0
(with .werft/ from main)

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Thanks for contributing, @Palanikannan1437! 🍊

  1. I'd rely on someone from the @gitpod-io/engineering-webapp to take a closer look at the code changes.
  2. You'll also need to sign a CLA[1] once before merging your first contribution. Cc @meysholdt

/hold

@Palanikannan1437
Copy link
Contributor Author

Palanikannan1437 commented Feb 1, 2023

Thanks for contributing, @Palanikannan1437! 🍊

  1. I'd rely on someone from the @gitpod-io/engineering-webapp to take a closer look at the code changes.
  2. You'll also need to sign a CLA[1] once before merging your first contribution. Cc @meysholdt

/hold

Hey @gtsiolis ,for sure... I've signed the CLA now from my side 🚀🚀

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

LGTM from WebApp 👋

@Palanikannan1437
Copy link
Contributor Author

Palanikannan1437 commented Feb 2, 2023

LGTM from WebApp 👋

Awesome!! Thanks so much for the super quick review @gtsiolis @geropl 🧡

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-dashboard-lint-fix-loginpage.0 because the annotations in the pull request description changed
(with .werft/ from main)

@gtsiolis
Copy link
Contributor

gtsiolis commented Feb 8, 2023

/werft run with-preview=true

👍 started the job as gitpod-build-dashboard-lint-fix-loginpage-fork.1
(with .werft/ from main)

@meysholdt meysholdt added cla: accepted ✔️ and removed do-not-merge/cla-pending CLA has not been signed labels Feb 14, 2023
@meysholdt
Copy link
Member

Thank you for singing the CLA @Palanikannan1437!

@gtsiolis
Copy link
Contributor

gtsiolis commented Feb 14, 2023

@Palanikannan1437 Could you squash commits in this one? 🏓

@Palanikannan1437 Palanikannan1437 force-pushed the dashboard-lint-fix-loginpage branch from d486970 to fa60c44 Compare February 14, 2023 13:28
@Palanikannan1437
Copy link
Contributor Author

Palanikannan1437 commented Feb 14, 2023

Hey @gtsiolis, for sure! I've squashed them into one😄

@gtsiolis
Copy link
Contributor

gtsiolis commented Feb 14, 2023

Thanks @Palanikannan1437! Removing hold as this has been already approved from the @gitpod-io/engineering-webapp team in #16158 (review) and CLA has been signed in #16158 (comment).

/werft run

👍 started the job as gitpod-build-dashboard-lint-fix-loginpage-fork.2
(with .werft/ from main)

/hold

@gtsiolis
Copy link
Contributor

/unhold

@roboquat roboquat merged commit 3e40a62 into gitpod-io:main Feb 14, 2023
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.

[dashboard] Lint warnings
5 participants