-
-
Notifications
You must be signed in to change notification settings - Fork 56
Create io.github.wpkelso.slate.json #575
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congrats on your first app! It's so good it makes me want to cry. You did everything like almost perfectly, best practice, so good. Honestly a joy to see your submission. I'm so proud 🩷
There's just a couple of blockers:
Looks like a copy paste error, you appdata summary says "Mail" :) https://github.com/wpkelso/slate/blob/92f3db475907ada3abdc3e2005f27aa99cc691b9/data/slate.metainfo.xml.in#L9
Your screenshot URL is returning a 404 currently. Ah because it looks like you switched your screenshot to be a jpg. Remember your screenshot needs to have a transparent background
This stuff is optionally advice:
AppCenter can show the correct light or dark mode screenshot automatically instead of doing this slashy thing you should supply a separate dark and light mode screenshot and mark them with the environment tag: https://www.freedesktop.org/software/appstream/docs/chap-Metadata.html#tag-screenshots
I don't think your app actually needs --filesystem=xdg-documents since it uses the file portal to open and save files. You might want to double check if you can remove this permission
In your headerbar, there's actually a property to show window controls so you don't have to add them manually: https://valadoc.org/gtk4/Gtk.HeaderBar.show_title_buttons.html
In other apps with a split view we pack the headebar into a box manually because by default it goes across the whole top of the window. In your app, since you only have one view, you can just set your custom header as the titlebar instead of doing the blank titlebar trick. Unless you're intending to add a sidebar or something, then ignore me :)
This seems to be just copy/paste from Code? I don't think your app will actually have permission to do this
- Streamline headerbar containers - Remove hamstrung backup - Fix copy/paste errors in - Remove xdg-documents sandbox - Update screenshots
|
I will definitively be using this, mister otter :) |
danirabbit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more small thing! It looks like your screenshot URLs are 404ing because you tagged your release "v1.0.1" instead of "1.0.1". So if you update the tag we should be good to go :)
|
@danirabbit Tags should be good to go now |
danirabbit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic work! Congrats 🎉
Review Checklist
AppData
Flatpak