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

Feature/create doujin tiles for library view #16

Merged
merged 12 commits into from Mar 18, 2022

Conversation

CaptainBluBlu
Copy link
Collaborator

@CaptainBluBlu CaptainBluBlu commented Mar 14, 2022

I have done the design.
Used GridView and made cards + bling bling on them with an external package

Make sure to update yo dependencies ⬆️ 📅

PS : The view is still just for show. The dynamic list is implemented but commented out. Also need to make the card event (on tap to go to details page)

@CaptainBluBlu CaptainBluBlu linked an issue Mar 14, 2022 that may be closed by this pull request
Copy link
Owner

@wooneusean wooneusean left a comment

Choose a reason for hiding this comment

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

Overall, looks good, a few minor changes and its good to go.

lib/views/doujin/doujin_library/doujin_cards.dart Outdated Show resolved Hide resolved
lib/views/doujin/doujin_library/doujin_cards.dart Outdated Show resolved Hide resolved
lib/views/doujin/doujin_library/doujin_cards.dart Outdated Show resolved Hide resolved
lib/views/doujin/doujin_library/library_view.dart Outdated Show resolved Hide resolved
@wooneusean
Copy link
Owner

Besides those comments, can you make it so that the size of the image does not effect the size of the image box? Right now its a square because its 500x500 but if its a different shape, it will be all inconsistent. Preferably, make it one single size and make the image like object fit: cover in css u know.

- Naming Cards to Card
- Additional comma for readability
- Fixed size for image
@CaptainBluBlu
Copy link
Collaborator Author

CaptainBluBlu commented Mar 14, 2022

I have done the changes

@wooneusean you can check the latest commit

- Card new Design
- On click event link to details page
Copy link
Owner

@wooneusean wooneusean left a comment

Choose a reason for hiding this comment

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

Issues

  • Loading circle is not centered, its at the top left. Please make it centered with some margin top to make it go down abit.
  • First time load will call the seed function but does not stop the loading. (this is fine for production but debugging is kinda a hassle)
  • The cards are too close to the edges, please add some padding to the container.
  • The black gradient is not dark enough, the text needs to stand out more.
  • The star rating is too high, please use a different approach for placing it.
    Screenshot_20220317-203210.jpg

@wooneusean
Copy link
Owner

wooneusean commented Mar 17, 2022

I am done merging main into this branch, however, there is a problem. When I update the ratings, there is no change to the record. I have logged the output of the update of sembast but it shows all correct values. Please look into this for me, thanks.

Edit: Nevermind I found out the issue, I am simply dumb lmao.

@wooneusean wooneusean self-requested a review March 18, 2022 00:31
@wooneusean wooneusean merged commit 2770438 into main Mar 18, 2022
@wooneusean wooneusean deleted the feature/create-doujin-tiles-for-library-view branch March 18, 2022 01:03
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.

Create doujin tiles for library view
2 participants