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

Td 1166 feedback modal big five assessment #322

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

quangngd
Copy link
Contributor

src/components/report/candidate.js Outdated Show resolved Hide resolved
src/components/results/feedback/index.js Outdated Show resolved Hide resolved
src/components/results/feedback/index.js Outdated Show resolved Hide resolved
src/components/results/feedback/index.js Outdated Show resolved Hide resolved
src/components/results/feedback/modal/index.js Outdated Show resolved Hide resolved
src/components/results/feedback/modal/index.js Outdated Show resolved Hide resolved
src/components/results/feedback/modal/index.js Outdated Show resolved Hide resolved
src/components/results/feedback/modal/index.js Outdated Show resolved Hide resolved
src/components/results/feedback/modal/style.scss Outdated Show resolved Hide resolved
src/components/results/feedback/index.js Outdated Show resolved Hide resolved
Copy link
Member

@tomprats tomprats left a comment

Choose a reason for hiding this comment

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

Your updates look great! I think we're good until the backend is ready.

The only part I don't love is the awkward margin numbers like $buffer-lg * -16.75 0. We might want to use flex to center it. I think that code has been copied down since before there were better ways of centering

@quangngd
Copy link
Contributor Author

quangngd commented Mar 28, 2024

Your updates look great! I think we're good until the backend is ready.

The only part I don't love is the awkward margin numbers like $buffer-lg * -16.75 0. We might want to use flex to center it. I think that code has been copied down since before there were better ways of centering

Simplified the css rule

Copy link
Member

@tomprats tomprats left a comment

Choose a reason for hiding this comment

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

Things are looking good! But another thing we;d still want is some tests. Should just need to mock the assessment and http call and then have it render different questions and submit. Check out some other components for how we've done that in the past.

src/components/report/candidate.js Outdated Show resolved Hide resolved
src/lib/feedback-survey.js Outdated Show resolved Hide resolved
src/components/results/feedback/modal/index.js Outdated Show resolved Hide resolved
Copy link
Member

@tomprats tomprats left a comment

Choose a reason for hiding this comment

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

Things are almost there! Tests look good, refactor looks good, just a couple tweaks left

src/components/common/modal/index.js Outdated Show resolved Hide resolved
src/components/report/candidate.js Outdated Show resolved Hide resolved
src/components/results/feedback/index.js Outdated Show resolved Hide resolved
src/components/results/feedback/index.js Outdated Show resolved Hide resolved
src/components/results/feedback/index.js Outdated Show resolved Hide resolved
src/components/report/candidate.js Outdated Show resolved Hide resolved
src/components/results/feedback/modal/index.js Outdated Show resolved Hide resolved
@quangngd quangngd requested a review from tomprats April 10, 2024 01:41
Copy link
Member

@tomprats tomprats left a comment

Choose a reason for hiding this comment

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

Everything is looking great! Before we merge to master we'll want to:

  • Push to staging to run it by Josh
  • Bump the version (make sure you bump package.json and then run npm install to bump package-lock.json)
  • Make sure the feedback service goes out first

@quangngd quangngd requested a review from tomprats May 15, 2024 02:18
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.

None yet

2 participants