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
Choose a base branch
from

Conversation

Vinnl
Copy link
Contributor

@Vinnl Vinnl 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.

The rdflib typings have been updated, so these are no longer
necessary.
@Vinnl Vinnl requested a review from megoth June 13, 2019 09:09
* @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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@Vinnl
Copy link
Contributor Author

Vinnl commented Jun 13, 2019

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

image

Copy link
Contributor

@megoth megoth left a comment

Choose a reason for hiding this comment

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

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",
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

)
}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Vinnl Vinnl Jun 13, 2019

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@megoth
Copy link
Contributor

megoth 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
Copy link
Contributor

megoth 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants