Skip to content
This repository has been archived by the owner on Jan 12, 2023. It is now read-only.

fix(cleanup): fix skip to content link page refresh issue #616

Merged
merged 1 commit into from
Jun 1, 2021

Conversation

seanforyou23
Copy link
Contributor

This PR is mostly just some small housekeeping items. The link in quickstart for the example json content was out of date, I updated and also made it a relative path so it works as expected on forks.

I'm unsure what .env.example is supposed to point to?

I also noticed when selecting the SkipToContent link the whole page refreshes, this should be fixed now.

Lastly, the spinner just needed an accessible name
Screen Shot 2021-06-01 at 12 59 35 PM

@seanforyou23 seanforyou23 requested a review from a team June 1, 2021 18:31
@mturley
Copy link
Collaborator

mturley commented Jun 1, 2021

Ah, .env used to be how we pass the stuff that is now in meta.json (changed because it was simpler for the operator to use JSON), and for a while we still supported the dotenv plugin in case you wanted to persist those variables. Ended up removing it for some reason or another, so we should just remove references to the .env file in the README and instead describe that you can pass those environment variables via, well, the environment.

Also I see you're lacking maintainer rights, I'll fix that!

@konveyor-preview-bot
Copy link

🚀 Deployed Preview: http://konveyor-forklift-ui-pr-616-preview.surge.sh

Compare with current main branch: http://konveyor-forklift-ui-preview.surge.sh

Copy link
Contributor

@gildub gildub left a comment

Choose a reason for hiding this comment

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

Welcome onboard!
LGTM.

@mturley
Copy link
Collaborator

mturley commented Jun 1, 2021

@seanforyou23 looks like this is failing CI because of eslint errors. I forgot to mention we're using Prettier and enforcing its formatting using eslint-config-prettier and eslint-plugin-prettier. You can auto-fix these by running npm run format. I also just have Format on Save configured in VS Code using the Prettier extension, but that's a matter of preference. If you want it just for this project though, you can do that in workspace config.

Copy link
Collaborator

@mturley mturley left a comment

Choose a reason for hiding this comment

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

LGTM other than the lint errors!

update broken link in readme
ensure loading spinner has an accessible name
update tests
@sonarcloud
Copy link

sonarcloud bot commented Jun 1, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov
Copy link

codecov bot commented Jun 1, 2021

Codecov Report

Merging #616 (e44017d) into main (cbd5d39) will decrease coverage by 0.03%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #616      +/-   ##
==========================================
- Coverage   58.74%   58.71%   -0.04%     
==========================================
  Files         143      143              
  Lines        4727     4735       +8     
  Branches     1298     1299       +1     
==========================================
+ Hits         2777     2780       +3     
- Misses       1913     1918       +5     
  Partials       37       37              
Impacted Files Coverage Δ
src/app/common/components/LoadingEmptyState.tsx 100.00% <ø> (ø)
src/app/AppLayout/AppLayout.tsx 86.00% <50.00%> (-9.24%) ⬇️
src/app/utils/utils.ts 40.00% <50.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbd5d39...e44017d. Read the comment docs.

@mturley mturley merged commit 936b2eb into kubev2v:main Jun 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants