make title field editable #509
Conversation
| className='bn f6 pl1 normal w-100' | ||
| defaultValue={editValue || title} | ||
| onKeyUp={ev => this.handleKeyup(ev)} | ||
| innerRef={input => { |
There was a problem hiding this comment.
why innerRef? check here - styled-components/styled-components#102
There was a problem hiding this comment.
If its hard enough to make a note in a github comment, then its maybe hard/rare enough to add a comment to the code?
aks-
force-pushed
the
add/edit-dat-title
branch
from
89f996e
to
9f7a017
Compare
app/actions/index.js
Outdated
|
|
||
| export const updateTitle = (key, path, editValue) => { | ||
| const filePath = `${path}/dat.json` | ||
| readFile(filePath) |
There was a problem hiding this comment.
Might be better to use dat-json ?
There was a problem hiding this comment.
@martinheidegger it requires archive for it to work https://github.com/joehand/dat-json/blob/master/index.js#L14 which we are not saving in our dat(s). Additionally in the prior version they did almost same as what we are doing right now in this pr - https://github.com/dat-land/dat-desktop/blob/master/lib/dat-json.js
There was a problem hiding this comment.
Not sure 👍 or not but can be fixed in a future pr.
| className='bn f6 pl1 normal w-100' | ||
| defaultValue={editValue || title} | ||
| onKeyUp={ev => this.handleKeyup(ev)} | ||
| innerRef={input => { |
There was a problem hiding this comment.
If its hard enough to make a note in a github comment, then its maybe hard/rare enough to add a comment to the code?
app/containers/title-field.js
Outdated
| dispatch(updateTitle(key, path, editValue)), | ||
| activateTitleEditing: (title, editable) => | ||
| dispatch(activateTitleEditing(title)), | ||
| editTitle: title => dispatch(editTitle(title)), |
There was a problem hiding this comment.
I was confused by the name editTitle. Maybe directTitleInput or so? Tbh. editTitle seems like the only action that doesn't make sense while the others seem to fit perfectly.
There was a problem hiding this comment.
changed the name to updateTemporaryTitleValue
aks-
force-pushed
the
add/edit-dat-title
branch
from
9f7a017
to
176a795
Compare
| this.titleInput = input | ||
| }} | ||
| /> | ||
| {editValue === title ? ( |
There was a problem hiding this comment.
this is only place where we need the editValue and that's why we are keeping it in the state using the updateTemporaryTitleValue, if we can get rid of it somehow we can have just one flag in state isTitleUnderEdit and get rid of updateTemporaryTitleValue action and UPDATE_TEMPORARY_TITLE_VALUE event as well. Maybe when we refactor out this inline title input field to a package...
app/actions/index.js
Outdated
|
|
||
| export const updateTitle = (key, path, editValue) => { | ||
| const filePath = `${path}/dat.json` | ||
| readFile(filePath) |
There was a problem hiding this comment.
should use async/await
There was a problem hiding this comment.
also this fails if there's no dat.json yet
There was a problem hiding this comment.
A. Currently it fails silently (no catch handler) and it returns the function before the writing is finished. Not sure if writing this async would look so cool:
(async function () {
const metadata = JSON.parse(await readFile(filePath)
await writeFile(filePath, JSON.stringify({... metadata, title: editValue})
})()Not soo much better, but I agree: slightly.
B. I didn't mention the fail of missing dat.json because in practice the duration of not-having a dat.json is very short. (milliseconds?) - quicker than any user can edit a title. Also I am not sure how the whole functionality should work: If there is no dat.json - can you edit the title? What if the dat.json was deleted while editing or we had an write error? Should there be an alert? Shouldn't this be part of the "lock" discussion (-> future work)?
There was a problem hiding this comment.
I thought about it failing in the way if someone deletes the file in middle of editing, what will happen. But was struggling with design on how and where to show the alert, maybe alert box like when we delete the dat. Should we create an issue or add it to todo list?
app/components/table-row.js
Outdated
| const Row = ({ dat, shareDat, onDeleteDat, inspectDat, onTogglePause }) => ( | ||
| <Tr | ||
| onClick={ev => { | ||
| if (ev.target.tagName === 'SVG' || ev.target.tagName === 'use') return | ||
| if ( |
There was a problem hiding this comment.
can be refactored using an array: if (['SVG', 'use', 'INPUT'].includes(ev.target.tagName))
app/containers/title-field.js
Outdated
| return mapStateToProps | ||
| } | ||
|
|
||
| const mapDispatchToProps = dispatch => ({ |
There was a problem hiding this comment.
this is hard to read, maybe let's add the { return ... } around the function block?
| import React, { Component } from 'react' | ||
| import styled from 'styled-components' | ||
| import Icon from './icon' | ||
| import { Plain as PlainButton, Green as GreenButton } from './button' |
There was a problem hiding this comment.
this can be written shorter using
import { * as Button } from './button'and then
<Button.Green />There was a problem hiding this comment.
I'd prefer not to have any * in the import statements as that makes it unclear which of the parts of ./button are required in the file by just looking at the header.
There was a problem hiding this comment.
I see that point, but for buttons this is literally just different colors, so you don't get relevant information out of this
There was a problem hiding this comment.
?! Just reading it like this: I get the information what buttons are used, because only those two buttons are referenced. If I don't use one of the two buttons the linter will complain about an unused reference.
There was a problem hiding this comment.
Hm, not sure if I should go ahead with this one? I personally like PlainButton than Button.plain 🤔
There was a problem hiding this comment.
ok, then 2>1 => PlainButton it is :)
app/reducers/index.js
Outdated
| @@ -15,7 +15,11 @@ const defaultState = { | |||
| up: 0, | |||
| down: 0 | |||
| }, | |||
| inspect: { key: null } | |||
| inspect: { key: null }, | |||
| titleUnderEdit: { | |||
There was a problem hiding this comment.
this name isn't clear to me, under is mostly used for location.
Can we call this for example titleEditInPlace?
There was a problem hiding this comment.
@juliangruber yeah titleEditInPlace sounds better. Making that change now....
There was a problem hiding this comment.
My - horrible - default approach would be currentlyEditedTitle. To me both titleEditInPlace and titleUnderEdit are almost equal so I leave it up to you. Other ideas: focussedTitle or titleEditorField
There was a problem hiding this comment.
currentlyEditedTitle works for me too, it's natural +1 but different from all the other state names -1
There was a problem hiding this comment.
Since titleEditInPlace is already in this PR. lets go with that 😆
No description provided.