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.

Sign up for GitHub

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

Jump to bottom

feat: add collection vote table #525

Merged
merged 23 commits into from
Dec 5, 2023
Merged

feat: add collection vote table #525

merged 23 commits into from
Dec 5, 2023

Conversation

shahin-hq
Copy link
Contributor

@shahin-hq shahin-hq commented Nov 30, 2023 •
edited
Loading

Summary

Closes: https://app.clickup.com/t/86dqnpf51

Design spec: https://www.figma.com/file/2Rr7zzoeGGiHYsROvfHaGe/Dashboard?node-id=23916%3A85080&mode=dev

Checklist

  • I checked my UI changes against the design and there are no notable differences
  • I checked my UI changes for any responsiveness issues
  • I checked my (code) changes for obvious issues, debug statements and commented code
  • I opened a corresponding card on Clickup for any remaining TODOs in my code
  • I added a short description on how to test this PR (if necessary)
  • I added a storybook entry for the component that was added (if necessary)
  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

@shahin-hq shahin-hq marked this pull request as ready for review December 1, 2023 14:29
shahin-hq
shahin-hq commented Dec 1, 2023
View reviewed changes
resources/js/Pages/Collections/Components/CollectionOfTheMonth/CollectionOfTheMonth.tsx Outdated
Copy link
Contributor Author

@shahin-hq shahin-hq Dec 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Renamed parent dir to CollectionVoting

shahin-hq
shahin-hq commented Dec 1, 2023
View reviewed changes
resources/js/Pages/Collections/Components/CollectionOfTheMonth/index.tsx Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this to the index file, it is good to have this in the index file because of the layout.

shahin-hq
shahin-hq commented Dec 1, 2023
View reviewed changes
resources/js/Pages/Collections/Components/CollectionVoting/VoteCollections.tsx
volumeDecimals?: number;
votes?: number;
index: number;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dummy interface, should be linked with backend data

@alfonsobries alfonsobries self-assigned this Dec 1, 2023
alfonsobries
alfonsobries reviewed Dec 1, 2023
View reviewed changes
Copy link
Contributor

@alfonsobries alfonsobries left a comment •
edited
Loading

Choose a reason for hiding this comment

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

there is an issue with the spacing below the table, but that comes from the original branch (caused by the chart), will add a comment on the ticket for the responsive version of the chart so can be handled there
image

alfonsobries
alfonsobries requested changes Dec 1, 2023
View reviewed changes
Copy link
Contributor

@alfonsobries alfonsobries left a comment

Choose a reason for hiding this comment

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

There seems to be an issue with the vote height, (at least on mobile, didnt check the rest), design the height is 80px but implementation is 76px

image image

@alfonsobries alfonsobries marked this pull request as draft December 1, 2023 15:34
@shahin-hq
Copy link
Contributor Author

@alfonsobries I fixed the VotingCollection item paddings, however, I'd not check it's height as there are some discrepancies in the design spec:

image

@shahin-hq shahin-hq marked this pull request as ready for review December 1, 2023 17:12
shahin-hq and others added 5 commits December 1, 2023 21:21
@shahin-hq
feat: add table
cb5511f
@shahin-hq
Merge remote-tracking branch 'origin/feat/collections-vote-table' int…
c412cf4 
…o feat/collections-vote-table

# Conflicts:
#	resources/js/Pages/Collections/Components/CollectionVoting/VoteCollections.tsx
@shahin-hq
style: resolve style guide violations
8c8af38
@shahin-hq
feat: add table
d2402eb
@shahin-hq
Merge remote-tracking branch 'origin/feat/collections-vote-table' int…
28900f2 
…o feat/collections-vote-table
@ItsANameToo ItsANameToo added this to the 0.13.0 milestone Dec 4, 2023
@ItsANameToo
Copy link
Contributor

some conflicts in the meantime @shahin-hq

@shahin-hq
Merge branch 'feat/collections-overview' into feat/collections-vote-t…
d930bff 
…able

# Conflicts:
#	resources/js/I18n/Locales/en.json
#	resources/js/Pages/Collections/Components/CollectionOfTheMonth/CollectionOfTheMonth.tsx
#	resources/js/Pages/Collections/Components/CollectionOfTheMonth/VoteCollection.tsx
#	resources/js/Pages/Collections/Index.tsx
@shahin-hq
wip
96f17f6
@shahin-hq
wip
03e5b95
@crnkovic crnkovic self-assigned this Dec 4, 2023
crnkovic
crnkovic suggested changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@crnkovic crnkovic left a comment

Choose a reason for hiding this comment

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

It appears there is no transition or focus styles on rows in the table, plus there is some mismatch in the border around the image on dark mode:

Implementation:
Screenshot 2023-12-04 at 14 57 23

Design:
Screenshot 2023-12-04 at 14 58 21

@crnkovic crnkovic marked this pull request as draft December 4, 2023 13:59
@shahin-hq
Copy link
Contributor Author

@crnkovic There is no focus styling in the design spec, that is why I used hover color for it, please check. I also fixed border color issue.

@shahin-hq shahin-hq marked this pull request as ready for review December 4, 2023 14:49
@ItsANameToo
Copy link
Contributor

small follow up in https://app.clickup.com/t/86dqqdr5p

@ItsANameToo ItsANameToo merged commit 35da574 into feat/collections-overview Dec 5, 2023
16 checks passed
@ItsANameToo ItsANameToo deleted the feat/collections-vote-table branch December 5, 2023 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ItsANameToo ItsANameToo ItsANameToo approved these changes

@crnkovic crnkovic crnkovic approved these changes

@samharperpittam samharperpittam Awaiting requested review from samharperpittam

@boldninja boldninja Awaiting requested review from boldninja

@alfonsobries alfonsobries Awaiting requested review from alfonsobries

Assignees

@crnkovic crnkovic

@alfonsobries alfonsobries

Labels
feature
Projects
None yet
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.
4 participants
@shahin-hq @ItsANameToo @crnkovic @alfonsobries