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

Stampy redesign 598 #638

Closed
wants to merge 5 commits into from

Conversation

zarSou9
Copy link
Contributor

@zarSou9 zarSou9 commented May 24, 2024

This is the complete 598 task regarding the feedback ui with all requirements met except the backend posting to the database when a user up or down votes a response. In the index file under feedback, I've added a 'sendVote' function which is where this should happen.

I've also installed and set up tailwind as a dependency and used it to do the styling for. I encourage others to use it. This required also the dependencies "autoprefixer" and "postcss"

Also, I've installed "react-router-dom" and "@cloudflare/kv-asset-handler" to the dependencies list because the lack of them was causing reference errors. I guess the devs here have those globally installed or something? If it doesn't make sense to add those, you can remove them, but make sure you keep the tailwind, autoprefixer, and postcss, because my code depends on them.

@zarSou9
Copy link
Contributor Author

zarSou9 commented May 24, 2024

feedback.mov

@zarSou9
Copy link
Contributor Author

zarSou9 commented May 24, 2024

Note that this pr also includes the matomo tracking commit made earlier bc I cherry picked it after it was made


const thanksRef = useRef<HTMLDivElement | null>(null)

function sendVote(v: boolean) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

the Action component handles that for you. It already was sending thumb up/down feedback to the backend. At least for articles with pageid - chatbot replies don't, as it's not yet certain where that should go. This bit here is responsible for making sure the backend is properly handled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I deleted the action component and kinda went from scratch is because I spent like 40 mins trying to get the styling working so it fit with the design on the figma, but it was just too difficult and I didn't want to mess up other parts of the site which were using the Action component. I've now copied the logic for posting to the backend into the sendVote function, but would still like for you to verify it.

'p-1 rounded-[4px] relative ' +
(vote === true ? 'bg-[#EDFAF9]' : vote === undefined ? 'hover:bg-[#F9FAFC]' : '')
}
onMouseEnter={() => setUpHover(true)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not just do this in css? The :hover pseudo class is for that - no point in using js where css can do the job better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are totally right, I've just fixed it

@mruwnik
Copy link
Collaborator

mruwnik commented May 24, 2024

you're going to have to run npm run lint:fix before the PR check will pass

@zarSou9
Copy link
Contributor Author

zarSou9 commented May 24, 2024

I've just made a commit incorporating your feedback. Also, in it i've improved the UI of the edit and feedback buttons at the footer of articles so they fit well.

@zarSou9 zarSou9 closed this May 25, 2024
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.

2 participants