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

feat: fixed toast ui #862

Merged
merged 5 commits into from
Jun 27, 2023
Merged

feat: fixed toast ui #862

merged 5 commits into from
Jun 27, 2023

Conversation

Oluwaseyi3
Copy link
Contributor

Problem

The icon wasn't aligned with the text in the toast UI. It also needed some padding

Changes Made

I worked on the UI based the Figma model and reconciled the differences with Tailwind CSS

Screenshots

[Include screenshots, if relevant, to help reviewers understand the changes you made.]

toast

Reviewers

@mlabouardy

Copy link
Collaborator

@mlabouardy mlabouardy left a comment

Choose a reason for hiding this comment

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

Thanks @Oluwaseyi3, a small change and we should be good :)

<div>
<p className="text-sm font-medium">{title}</p>
<div className="p-3">
<p className="pb-2 text-xl font-medium">{title}</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you revert to text-sm as the title is quite huge :)
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright. On it Mohamed!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @mlabouardy I set it to text-lg and it looks perfectly okay for a title. Just a bit bigger than the text body. You can check it out.

hh

If you still want it to be reviewed pls don't hesitate to notify me

Copy link
Collaborator

Choose a reason for hiding this comment

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

all good then :)

@mlabouardy mlabouardy self-requested a review June 21, 2023 17:20
Copy link
Collaborator

@mlabouardy mlabouardy 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 the changes @Oluwaseyi3 :)

@mlabouardy
Copy link
Collaborator

Thanks for the changes @Oluwaseyi3 :)

seems like CI failed due to package.json changes, could you check that @Oluwaseyi3
https://github.com/tailwarden/komiser/actions/runs/5334667306/jobs/9672597140?pr=862

@Oluwaseyi3
Copy link
Contributor Author

Thanks for the changes @Oluwaseyi3 :)

seems like CI failed due to package.json changes, could you check that @Oluwaseyi3 https://github.com/tailwarden/komiser/actions/runs/5334667306/jobs/9672597140?pr=862

Alright. Will correct that asap

@Oluwaseyi3
Copy link
Contributor Author

Thanks for the changes @Oluwaseyi3 :)

seems like CI failed due to package.json changes, could you check that @Oluwaseyi3 https://github.com/tailwarden/komiser/actions/runs/5334667306/jobs/9672597140?pr=862

Alright. Will correct that asap

Thanks for the changes @Oluwaseyi3 :)

seems like CI failed due to package.json changes, could you check that @Oluwaseyi3 https://github.com/tailwarden/komiser/actions/runs/5334667306/jobs/9672597140?pr=862

Alright. Will correct that asap

@mlabouardy Pls you can check it out now. I updated the package.json. Let me know if the test cli passes

@Oluwaseyi3
Copy link
Contributor Author

Thanks for the changes @Oluwaseyi3 :)

seems like CI failed due to package.json changes, could you check that @Oluwaseyi3 https://github.com/tailwarden/komiser/actions/runs/5334667306/jobs/9672597140?pr=862

Alright. Will correct that asap

Thanks for the changes @Oluwaseyi3 :)

seems like CI failed due to package.json changes, could you check that @Oluwaseyi3 https://github.com/tailwarden/komiser/actions/runs/5334667306/jobs/9672597140?pr=862

Alright. Will correct that asap

@mlabouardy Pls you can check it out now. I updated the package.json. Let me know if the test cli passes

Hi @mlabouardy just noticed it failed once more. I’ve installed a new package.lock. This one is in sync with the develop branch. This would work now

Copy link
Contributor

@ShubhamPalriwala ShubhamPalriwala left a comment

Choose a reason for hiding this comment

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

Hey @Oluwaseyi3, I can still see a lot of package-lock.json changes in the git diff hence why the build is failing due to dependency mismatch.

One suggestion for a cleaner and quicker fix would be to remove the package-lock.json from the git tracking of this PR and just add and commit the dashboard/components/toast/Toast.tsx file, this should solve it all at once and the PR should be good to merge then

@Oluwaseyi3
Copy link
Contributor Author

Hey @Oluwaseyi3, I can still see a lot of package-lock.json changes in the git diff hence why the build is failing due to dependency mismatch.

One suggestion for a cleaner and quicker fix would be to remove the package-lock.json from the git tracking of this PR and just add and commit the dashboard/components/toast/Toast.tsx file, this should solve it all at once and the PR should be good to merge then

On it

@Oluwaseyi3
Copy link
Contributor Author

Hey @Oluwaseyi3, I can still see a lot of package-lock.json changes in the git diff hence why the build is failing due to dependency mismatch.

One suggestion for a cleaner and quicker fix would be to remove the package-lock.json from the git tracking of this PR and just add and commit the dashboard/components/toast/Toast.tsx file, this should solve it all at once and the PR should be good to merge then

All done @ShubhamPalriwala

@mlabouardy mlabouardy merged commit fcf778f into tailwarden:develop Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants