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

[Work in progress] Separation of concerns using React - TrustedApplications #120

Open
wants to merge 2 commits into
base: soc-react
from

Conversation

Projects
None yet
2 participants
@Vinnl
Copy link
Collaborator

commented Jun 13, 2019

I've separated this out from #117 to highlight only what is new compared to that. Like that PR, this is basically replicating the approach from #116 using React.

Vinnl added some commits Jun 13, 2019

Remove redundant casts
The rdflib typings have been updated, so these are no longer
necessary.

@Vinnl Vinnl requested a review from megoth Jun 13, 2019

* @param serialisedMode The full IRI of a mode
* @returns A plain text string representing that mode, i.e. 'read', 'append', 'write' or 'control'
*/
export function deserialiseMode (serialisedMode: $rdf.NamedNode, ns: Namespaces): Mode {

This comment has been minimized.

Copy link
@Vinnl

Vinnl Jun 13, 2019

Author Collaborator

@megoth Do you know if there's a built-in way to remove the namespace from an IRI?

This comment has been minimized.

Copy link
@megoth

megoth Jun 13, 2019

Collaborator

You mean something like https://www.w3.org/ns/auth/acl#Read - namespace = Read?

This comment has been minimized.

Copy link
@Vinnl

Vinnl Jun 13, 2019

Author Collaborator

Yes, exactly.

This comment has been minimized.

Copy link
@megoth

megoth Jun 14, 2019

Collaborator

Not that I'm aware of, no, but shouldn't be to difficult to create

@Vinnl

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

Btw, this is what it now looks like, given that the style guide is active now:

image

@megoth
Copy link
Collaborator

left a comment

I'm liking the general direction this is going! Good use of container-view pattern. It's not perfect, but I think it's a big step in the right direction.

I want to finish my Vue-experiment, and then we'll try to wrap this up.

@@ -43,7 +43,7 @@ Array [
Statement {
"object": NamedNode {
"termType": "NamedNode",
"value": "http://www.w3.org/ns/auth/acl#Read",
"value": "http://www.w3.org/ns/auth/acl#read",

This comment has been minimized.

Copy link
@megoth

megoth Jun 13, 2019

Collaborator

Nitpicking, but this should not be the case - Read (and the other modes) are classes, so should start with capital letters

This comment has been minimized.

Copy link
@Vinnl

Vinnl Jun 13, 2019

Author Collaborator

That's exactly the kind of nitpicking I need, otherwise I'd never learn. Is a "class" an RDF concept, and if so, how do I know when something is a class?

(This is also probably wrong at many other places.)


const addOrEditApp = (origin: string, modes: Mode[]) => {
const result = new Promise<void>((resolve) => {
const deletions = getStatementsToDelete($rdf.sym(origin), props.subject, props.store, ns)

This comment has been minimized.

Copy link
@megoth

megoth Jun 13, 2019

Collaborator

I'm not a big fan of this level of scoping =/ (This is where I like to rely on classes instead, which of course introduces some challenges of its own.) Just wanted to mention it.

This comment has been minimized.

Copy link
@Vinnl

Vinnl Jun 13, 2019

Author Collaborator

Me neither, didn't think too long about this. Ideally, the updater would return a Promise itself. (Well, there's actually code that does, but it's not quite clear to me yet when - might be an avenue to pursue. If we do go ahead with this, I'll see if I can improve this.)

let panes
let UI: SolidUi

if (nodeMode) {

This comment has been minimized.

Copy link
@megoth

megoth Jun 13, 2019

Collaborator

Know that we have to do this wrt testing, but also would like a better approach at some point.

This comment has been minimized.

Copy link
@Vinnl

Vinnl Jun 13, 2019

Author Collaborator

That's not there for testing - it's primarily there because it already was there; see here.

I think it may have to do with having been a Firefox extension or something? Perhaps Tim knows more about it.

* @param serialisedMode The full IRI of a mode
* @returns A plain text string representing that mode, i.e. 'read', 'append', 'write' or 'control'
*/
export function deserialiseMode (serialisedMode: $rdf.NamedNode, ns: Namespaces): Mode {

This comment has been minimized.

Copy link
@megoth

megoth Jun 13, 2019

Collaborator

You mean something like https://www.w3.org/ns/auth/acl#Read - namespace = Read?

)
}

const NewApplication: React.FC<{ onSave: AddOrUpdate }> = (props) => {

This comment has been minimized.

Copy link
@megoth

megoth Jun 13, 2019

Collaborator

Hmm, perhaps better conceptually with two components, but adds a bit of overhead in terms of maintenance =/

This comment has been minimized.

Copy link
@Vinnl

Vinnl Jun 13, 2019

Author Collaborator

I thought it'd make it easier to read, but I'm certainly open to inlining it.

return into.concat(app)
}

return into.slice(0, index)

This comment has been minimized.

Copy link
@megoth

megoth Jun 13, 2019

Collaborator

Wouldn't it be better to use Array.prototype.splice?

This comment has been minimized.

Copy link
@Vinnl

Vinnl Jun 13, 2019

Author Collaborator

I usually go for methods that do not modify their input parameters, but sure, splice would work too.

@megoth

This comment has been minimized.

Copy link
Collaborator

commented Jun 14, 2019

This relates to #72

@Vinnl Vinnl changed the title [Work in progress] Separation of concerns using - TrustedApplications [Work in progress] Separation of concerns using React - TrustedApplications Jun 14, 2019

@megoth

This comment has been minimized.

Copy link
Collaborator

commented Jun 22, 2019

We will merge soc-react and soc-react-trustedApps into master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.