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

#31 Refactor to Components #32

Merged
merged 28 commits into from
May 29, 2024
Merged

#31 Refactor to Components #32

merged 28 commits into from
May 29, 2024

Conversation

alvinosh
Copy link
Collaborator

This PR includes a large chunk of refactor to the sveltekit codebase, as well as support for multiple tags, and image deletion

@prokawsar
Copy link
Collaborator

Hi @alvinosh
Some points:

  1. You have added routes for viewing images which BREAKS the image tab feature, opening an image should open in a new tab with its name. Using routing is not necessary here, so you can remove it.
  2. We had our delete modal already, we could reuse it and add some functionality for permanent delete.
  3. Delete UI doesn't match the existing UI schema, so it's better to confirm with Sona.
  4. For the trash icon, we don't need png, we can use the existing fontawesome trash icon with modified size and color on it.
Screenshot 2024-04-27 at 7 22 47 AM
  1. While short by tag, it should show all images initially, but it shows nothing.
    https://ibb.co/Sfm893M
  2. A bunch of store props has been deleted, where selectedImage has a crucial impact, we will need it while editing the image.
  3. Next and prev functionality for gallery view does not seem to work as expected. Having indexing issues.
    https://ibb.co/bX4c7bp

@kirillt
Copy link
Member

kirillt commented Apr 28, 2024

Something new with macOS build 🤔

image

@kirillt kirillt closed this May 12, 2024
@kirillt kirillt reopened this May 12, 2024
@kirillt
Copy link
Member

kirillt commented May 12, 2024

@alvinosh

  • New .app still can't be launched on my macOS, .app from main was fine

  • We should bring back the old deletion modal. We don't need 2 options: defaulting to trash can is fine for MVP, and in post-MVP we'll design proper choice with remembering/resetting

  • We need to discuss/address these points made by Kawsar:

    • 5. While sort by tag, it should show all images initially, but it shows nothing: https://ibb.co/Sfm893M
    • 6. A bunch of store props has been deleted, where selectedImage has a crucial impact, we will need it while editing the image.
    • 7. Next and prev functionality for gallery view does not seem to work as expected. Having indexing issues. https://ibb.co/bX4c7bp
  • @prokawsar I'm not so sure about tabs. We need to discuss with Sona if tabs is a good thing, probably tab support should be moved to post-MVP

@kirillt
Copy link
Member

kirillt commented May 23, 2024

@prokawsar @ryabovpavel could you check this PR one last time?

@prokawsar
Copy link
Collaborator

@alvinosh
In this PR, multiple tag selection is a useful feature.
I can see some improvements.

  • But still, I can reproduce opening the delete modal on an empty gallery.
    Steps to follow
  1. Load images
  2. Select any image with one click
  3. Select sort by tag
  4. Now, the empty gallery will show
  5. Click delete from the right top side
  6. And click yes, it will delete your previously selected image
  • And is there any issue with using stores values in Tauri?

  • You added a new component DeleteModal. That is a duplicate of QuestionModal component, which can be reused with some modification.

Copy link
Collaborator

@prokawsar prokawsar left a comment

Choose a reason for hiding this comment

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

  • Multiple tab support is not working as previously. That one might to discuss with Sona.
  • Tags are not stored in localStorage anymore. Removing a tag from an image will remove that tag from tag list too.

@ryabovpavel ryabovpavel added the QA passed The pullrequest has been reviewed and is ready for merge label May 28, 2024
@kirillt kirillt merged commit 9bd2f77 into main May 29, 2024
3 checks passed
@kirillt
Copy link
Member

kirillt commented May 29, 2024

Thanks everyone, I think this PR is a good example of team work 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA passed The pullrequest has been reviewed and is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants