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.

Sign up for GitHub

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

Jump to bottom

Clean up #17

Merged
merged 4 commits into from
Dec 19, 2021
Merged

Clean up #17

merged 4 commits into from
Dec 19, 2021

Conversation

NoelDeMartin
Copy link
Member

A bit of cleaning up after #16, @bourgeoa let me know if you have any comments.

Other than some formatting, one important change I've made is declaring the schema prefix as schemaorg. It seems like the default declaration is using http://, so our app will not work since it's using https://. I opened an issue to talk about that here: solid/solid-namespace#21 But for this repository, I think it's easier to just declare it differently and avoid confusing people with explanations.

Also, it seems like the CSS instructions no longer work, but I'm not sure if something's been broken on their part or I'm doing something wrong. I don't know if we should change the instructions, but for now I opened an issue about that: CommunitySolidServer/CommunitySolidServer#1102

bourgeoa
bourgeoa reviewed Dec 18, 2021
View reviewed changes
Copy link
Contributor

@bourgeoa bourgeoa left a comment

Choose a reason for hiding this comment

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

For the rest seems good.
See you used the getPrefix()
everything seems to work locally.

The only problem is that if you do not logout from a solid app (for example solid-rest-api) and got to the other one (solid-file-client) you are redirected to the previous one (solid-rest-api).

solid/solid-file-client/index.html Outdated
@@ -9,7 +9,7 @@
</head>

<body>
<h1>Solid-File-Client Hello World</h1>
<h1>Solid Hello World</h1>
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not understand why you changed the name here. When going around the pages You are not sure to know where you are except reading the page content.

Suggested change
<h1>Solid Hello World</h1>
<h1>Solid-file-client Hello World</h1>

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought the name was too long, but ok I agree it's not a great idea. I added the names back in parentheses.

index.html Outdated
@@ -20,7 +20,7 @@ <h1>0data Hello World</h1>
<ul>
<li><a href="./fission/">Fission</a></li>
<li><a href="./remotestorage/">remoteStorage</a></li>
<li><a href="./solid/">Solid</a></li>
<li><a href="./solid/solid-rest-api/">Solid</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

You choose to present solid-rest-api as the solid reference. I would not have made this choice

Suggested change
<li><a href="./solid/solid-rest-api/">Solid</a></li>
<li><a href="./solid/">Solid</a></li>

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, we can argue which one is the best for beginners, but I think we must choose one. The idea is to provide a simple example for people to get started, if the first thing we show them is a list of examples to choose from, that will confuse them more than anything.

I think the one using the REST API is the best one, because it's the one using the least libraries which was the initial motivation of this hello world. But I guess that's up for debate. As @rosano mentioned, maybe we want to choose the one that is more similar to the other protocols.

Copy link
Member

Choose a reason for hiding this comment

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

I've said before that I prefer library versions as the primary one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I've set the solid-file-client as the default example.

solid/solid-rest-api/solid.js
Comment on lines +255 to 258
// WebIds may declare more than one storage url, so in a real application you should
// ask which one to use if that happens. In this app, in order to keep it simple, we'll
// just use the first one. If none is declared in the profile, we'll search for it.
storageUrl: storageQuad?.object.value || await findUserStorage(webId),
Copy link
Contributor

@bourgeoa bourgeoa Dec 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I would have made a different choice because webIds Must not declare a pim:storage. But will not argue.
On the contrary it is a MUST to have a header with pim:Storage.

I just see that https://solidproject.org/TR/protocol v0.9 has been published

I don't know if you know this document about databrowser conventions (recently rediscovered) : https://github.com/solid/solid-panes/blob/main/Documentation/conventions.md

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess pim:storage for webIds is not in the spec, but it is well used in practice, right? In case it's missing, we fall back to finding the storage according to the spec so I think that's ok. I don't see a reason why we wouldn't use a pim:storage that is declared in the profile.

@NoelDeMartin
Copy link
Member Author

See you used the getPrefix()

Yeah, I think it's a more idiomatic way to showcase what the solid-file-client library can do :). It was a bummer though that using it to override schema didn't work, so I had to make up a name with schemaorg. I would expect that calling it would override the default, but I guess that's not how it works.

The only problem is that if you do not logout from a solid app (for example solid-rest-api) and got to the other one (solid-file-client) you are redirected to the previous one (solid-rest-api).

That's right, I hadn't noticed that problem. But that already exists before this PR so I guess we can merge it anyways. Maybe we should open a separate issue about this? Although I'm not sure what to do about it, without complicating in excess.

This was linked to issues Dec 18, 2021
make solid repo really multi-app #15
Closed
Code organisation for better first coder experience #3
Closed
@rosano
Copy link
Member

rosano commented Dec 18, 2021

Maybe we should open a separate issue about this? Although I'm not sure what to do about it, without complicating in excess.

If there are no bugs, I prefer merging as soon as possible and keeping scope as small as possible—lots of commits, issues, PRs that are self-contained…

@NoelDeMartin
Copy link
Member Author

Ok I've addressed all the points, so I'll go ahead and merge.

As per the issue with the authentication, I've opened a different issue #18.

@NoelDeMartin NoelDeMartin merged commit eb8b3db into 0dataapp:main Dec 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bourgeoa bourgeoa bourgeoa left review comments

@rosano rosano rosano left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

make solid repo really multi-app Code organisation for better first coder experience
3 participants
@NoelDeMartin @rosano @bourgeoa