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

WIP: app launcher #30

Merged
merged 41 commits into from
Mar 8, 2021
Merged

WIP: app launcher #30

merged 41 commits into from
Mar 8, 2021

Conversation

ylebre
Copy link
Member

@ylebre ylebre commented Dec 28, 2020

No description provided.

Copy link
Member

@Potherca Potherca left a comment

Choose a reason for hiding this comment

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

My concerns thus far

  1. It might be wise to add a version for Simply-Edit / Simply-Everything (either in a comment in the file or by changing the file name) to make it easier for future updates
  2. We might want to minify Simply-*.js as it is effectively vendor code (to discourage edits to it).
  3. I've not explicitly reviewed the PHP code for security issues, I intend to do a thorough review when this MR leaves WIP status.

@ylebre
Copy link
Member Author

ylebre commented Dec 29, 2020

1. It might be wise to add a version for Simply-Edit / Simply-Everything (either in a comment in the file or by changing the file name) to make it easier for future updates
2. We might want to minify Simply-*.js as it is effectively vendor code (to discourage edits to it).

Agreed - ideally I would like to grab these from a repo/vendor/thing much like the composer components. Maybe we should move them into js/vendor/simplyedit/ or something similar for now?

adding solid-auth-fetcher bundle
shouldn't be needed, as php-solid-auth also specifies that version, but somehow
composer appears to ignore that and goes for the latest version anyway
(which is not compatible with php-solid-auth)
poef and others added 4 commits January 30, 2021 15:39
…rk to generalize

currently fails to actually put() the file (403 forbidden)
use origin from apps definition
use permissions from apps definition
add user webId to the permissions
fix url for the acl doc.
Copy link
Member

@Potherca Potherca left a comment

Choose a reason for hiding this comment

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

The only concern I have is mixing var (rather than let) with const. But as that is trivial to clean up later, I am not much bothered right now.

Base automatically changed from master to main March 2, 2021 12:16
@ylebre ylebre merged commit 48b932c into main Mar 8, 2021
@ylebre ylebre deleted the app-launcher branch March 8, 2021 12:30
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.

3 participants