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: Add list view option to game library #115

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

marcusziade
Copy link
Contributor

@marcusziade marcusziade commented Jun 23, 2024

Close #57
Close #37

  • This PR adds a new list view option to the game library, allowing users to switch between the existing grid layout and a more compact list layout.

  • The new GameListRow component can later be refactored to share more stuff from GameCard.swift but IMO is fine as is for now.

  • Additionally, the layout config will persist as a user setting, as I think most want that.

  • Accessibility support for GameListRow

  • New localization strings

Grid view (currently the only option)

image

List view (New)

CleanShot 2024-06-22 at 19 01 39

@marcusziade marcusziade marked this pull request as ready for review June 23, 2024 01:41
@marcusziade marcusziade marked this pull request as draft June 23, 2024 01:54
@marcusziade marcusziade force-pushed the list-layout-library branch 4 times, most recently from 0f916c7 to 4773f18 Compare June 23, 2024 02:07
@marcusziade marcusziade marked this pull request as ready for review June 23, 2024 02:08
@marcusziade marcusziade force-pushed the list-layout-library branch 3 times, most recently from 3c2f53a to cf41528 Compare June 23, 2024 02:35
@marcusziade
Copy link
Contributor Author

marcusziade commented Jun 23, 2024

🤔 Not sure yet why CI fails...

Code compiles locally, as seen in the screenshots above. Is this something unexpected, @blackxfiied @jeremybosma ?

I've already updated the branch with latest main and fixed one call that no longer takes the option parameter.

@asxrow66
Copy link
Member

asxrow66 commented Jun 23, 2024

dm me on discord for adding this stuff to the docs and other work you've done, my discord name is cullenroberts

@vapidinfinity
Copy link
Member

🤔 Not sure yet why CI fails...

Code compiles locally, as seen in the screenshots above. Is this something unexpected, @blackxfiied @jeremybosma ?

I've already updated the branch with latest main and fixed one call that no longer takes the option parameter.

CI's been failling lately, unsure as to why; it may have something to do with the Package.resolved file, will check

@vapidinfinity
Copy link
Member

I'll merge these changes soon, but I want to make some changes and 'Mythicify™' the vertical list.

@marcusziade

This comment has been minimized.

@marcusziade
Copy link
Contributor Author

marcusziade commented Jun 23, 2024

Afaik these CI issues are due to commit #7400de5. There are compiler flags in the diff that didn't pass the CI check before it was merged. This branch wasn't failing until I updated it with main that had that broken commit merged to it.

Should be a simple revert to fix this and then redo that commit again later.

@marcusziade
Copy link
Contributor Author

marcusziade commented Jun 23, 2024

There's a setting in repo settings that requires CI to pass before a branch can merge. I'd suggest we enable that. There's also the auto-merge feature that complements the former.

@jeremybosma
Copy link
Contributor

This looks so unbelievably stunning, however with moving the filtering to a popover you should probably add this somewhere next to the filter icon in the top navigation. Not sure if we should merge because this entire filter bar will be gone then.

@jeremybosma
Copy link
Contributor

Will wait on those changes in both prs, looking awesome still. Let me know

@marcusziade
Copy link
Contributor Author

marcusziade commented Jun 23, 2024

We should merge both this and my other PRs. Then, iterate further.

None of my PRs conflict. I haven't done duplicate work. Just merge code and if there's a conflict, let me fix it, plz

@marcusziade
Copy link
Contributor Author

marcusziade commented Jun 23, 2024

Not sure if we should merge because this entire filter bar will be gone then.

Not sure what you mean. The filter system was already merged. This PR only adds the button that switches between grid and list layout.

@marcusziade
Copy link
Contributor Author

marcusziade commented Jun 23, 2024

@jeremybosma I'd prefer it if we'd have a little process to designs. So instead of me changing this PR with new design ideas on the fly, we'd merge this, close the issues asking for a list layout, then iterate further with improved UX. Also consider that I put a few hours of design work on this myself. I think it's very usable for an alpha, until we come up with better stuff. Also, this matter is out-of-scope from this PR. This is only about the list layout, not the filter toolbar.

What do you think?

@jeremybosma
Copy link
Contributor

@jeremybosma I'd prefer it if we'd have a little process to designs. So instead of me changing this PR with new design ideas on the fly, we'd merge this, close the issues asking for a list layout, then iterate further with improved UX. Also consider that I put a few hours of design work on this myself. I think it's very usable for an alpha, until we come up with better stuff. Also, this matter is out-of-scope from this PR. This is only about the list layout, not the filter toolbar.

What do you think?

I understand and agree with you fully, btw my request was not about the list layout which looks very good, i was more so talking about the filter toolbar because it will probably be too odd on non-maximized windows

@jeremybosma
Copy link
Contributor

I will design interface ideas for Mythic, any idea where to send them. Discord, Github or any other messaging platform? Imsg?

@vapidinfinity
Copy link
Member

@marcusziade

Afaik these CI issues are due to commit #7400de5. There are compiler flags in the diff that didn't pass the CI check before it was merged. This branch wasn't failing until I updated it with main that had that broken commit merged to it.

Should be a simple revert to fix this and then redo that commit again later.

the compiler flags shouldn't affect the build ci, and it's very strange that they do.
will look tomorrow since school holidays start then

@marcusziade
Copy link
Contributor Author

I will design interface ideas for Mythic, any idea where to send them. Discord, Github or any other messaging platform? Imsg?

depending on how advanced you wanna do it. i don't need pixel perfect designs but im able to give you that if you want.

if just sketches i think you can open an issue and attach screenshots?

Copy link
Member

Choose a reason for hiding this comment

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

Lacks "Mythicification", see DownloadsEvo.swift -> DownloadCard

Copy link
Contributor Author

@marcusziade marcusziade Jun 24, 2024

Choose a reason for hiding this comment

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

sure

can you explain the thinking behind the naming convention, just for context? what is "Mythicification"?

@marcusziade marcusziade force-pushed the list-layout-library branch from 7bee563 to b1028c7 Compare June 24, 2024 14:59
@marcusziade marcusziade force-pushed the list-layout-library branch from b1028c7 to cdd4b89 Compare June 24, 2024 17:58
@marcusziade marcusziade force-pushed the list-layout-library branch from cdd4b89 to 74b9a46 Compare June 24, 2024 18:00
@marcusziade marcusziade merged commit fcb0a42 into MythicApp:main Jun 24, 2024
2 checks passed
@marcusziade marcusziade deleted the list-layout-library branch June 24, 2024 18:04
vapidinfinity added a commit that referenced this pull request Jun 28, 2024
Refactoring of `GameListRow`, introduced in PR #115, is necessary.
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.

[Feature] Change library grid size [Feature] Library Visuals
4 participants