-
Notifications
You must be signed in to change notification settings - Fork 347
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 loading spinner beside add button #5129
Add loading spinner beside add button #5129
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
✅ Deploy Preview for care-egov-staging ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
{isLoading ? null : ( | ||
<div className="mb-2 mt-4"> | ||
{hasSkills ? ( | ||
<SkillsArray | ||
isLoading={isLoading} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CircularProgress we can add instead of null
import ButtonV2 from "../Common/components/ButtonV2"; | ||
import { SkillModel } from "./models"; | ||
|
||
const SELECT_SKILLS_COPY = "Select and add some skills"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we decleare constant Is this use again & again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We don't follow
.Components.tsx
file naming formats. - For the "literal strings not allowed" warning, you may make use of translations
const { t } = useTranslations();
and wrap the text like<Component label={t("add_skill")} />
. Also make sure the keyadd_skill
is present in the en locale files such asen/Common.json
or any other relevant context-specific locales. - Would be nice if the loading spinner was inside the button to the left of the "Add" text.
Working on this! |
@rithviknishad and @ayushjnv1 : I've made the changes as requested.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The button size changes between loading and not loading states.
Other than that LGTM.
Just feedback, Kindly make sure to avoid too many irrelevant changes. 90% of the changes present in this PR are not relevant to adding loading the spinner for add button. It would roughly take only 3~6 lines of change to achieve, but we have around 100+ lines of change here.
Hi, Feedback noted - I got carried away trying to get rid of the ESLINT error, but I understand that it wasn't in scope. I've made the change to the spinner and it's no longer changing in size. |
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LGTM |
Proposed Changes
Hi, @coronasafe/code-reviewers !
@coronasafe/care-fe-code-reviewers @coronasafe/code-reviewers
Merge Checklist
Add specs that demonstrate bug / test a new feature.
Update product documentation.
Ensure that UI text is kept in I18n files.
Prep screenshot or demo video for changelog entry, and attach it to issue.
Request for Peer Reviews
Completion of QA