[Work in progress] Separation of concerns using React - TrustedApplications#120
[Work in progress] Separation of concerns using React - TrustedApplications#120
Conversation
The rdflib typings have been updated, so these are no longer necessary.
| * @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 { |
There was a problem hiding this comment.
@megoth Do you know if there's a built-in way to remove the namespace from an IRI?
There was a problem hiding this comment.
You mean something like https://www.w3.org/ns/auth/acl#Read - namespace = Read?
There was a problem hiding this comment.
Not that I'm aware of, no, but shouldn't be to difficult to create
|
Btw, this is what it now looks like, given that the style guide is active now:
|
megoth
left a comment
megoth
left a comment
There was a problem hiding this 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.
| "object": NamedNode { | ||
| "termType": "NamedNode", | ||
| "value": "http://www.w3.org/ns/auth/acl#Read", | ||
| "value": "http://www.w3.org/ns/auth/acl#read", |
There was a problem hiding this comment.
Nitpicking, but this should not be the case - Read (and the other modes) are classes, so should start with capital letters
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Know that we have to do this wrt testing, but also would like a better approach at some point.
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
You mean something like https://www.w3.org/ns/auth/acl#Read - namespace = Read?
| ) | ||
| } | ||
|
|
||
| const NewApplication: React.FC<{ onSave: AddOrUpdate }> = (props) => { |
There was a problem hiding this comment.
Hmm, perhaps better conceptually with two components, but adds a bit of overhead in terms of maintenance =/
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Wouldn't it be better to use Array.prototype.splice?
There was a problem hiding this comment.
I usually go for methods that do not modify their input parameters, but sure, splice would work too.
|
This relates to #72 |
Vinnl
changed the title
|
We will merge soc-react and soc-react-trustedApps into master. |
2 participants
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.