Skip to content
This repository has been archived by the owner on Jan 6, 2022. It is now read-only.

Make table title field editable #501

Closed
wants to merge 1 commit into from

Conversation

aks-
Copy link
Collaborator

@aks- aks- commented Feb 4, 2018

No description provided.

updateTitle,
makeEditable,
editTitle,
deactivate
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like adding above props, any suggestions on how to refactor this?

onKeyUp={ev => this.handleKeypress(ev)}
ref={input => {
this.titleInput = input
}}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can i do it without having to use ref?

@aks- aks- mentioned this pull request Feb 4, 2018
@martinheidegger
Copy link
Collaborator

Why did you implement this yourself, rather than using an existing component such as https://github.com/kaivi/riek ?

@aks-
Copy link
Collaborator Author

aks- commented Feb 5, 2018

@martinheidegger Hm, that's good point.... I will go through that library and ones like that to see if they would be good fit and refactor it with them

@aks-
Copy link
Collaborator Author

aks- commented Feb 7, 2018

@martinheidegger I looked into it and libraries like this one for react. This one seem to be most mature, but there are certain problems with it when it comes to our usage. There is no way to tell if RIEInput is selected or clicked upon, hence we can't tell if we are already in editing mode and we should show the save button. Also this project doesn't look like maintained well. I saw a lot of PR(s) unanswered if we make a pr into that for the feature. For instance take a look at this one - kaivi/riek#65 (comment)

I think we should first get it working and write our own library to replace the refs eventually

@martinheidegger
Copy link
Collaborator

If it doesn't work, it doesn't work - thank you for giving it a try: It seems like riak needs a new maintainer. That being said: I think this needs a rebase. I found a few issues before that I didn't want to mention before a rewrite to riak - would be nice to add them to a rebased version.

@aks-
Copy link
Collaborator Author

aks- commented Feb 7, 2018

Yes I also found few, I am closing it and push a branch and make a pr again. Still working on it....

@aks- aks- closed this Feb 7, 2018
@martinheidegger
Copy link
Collaborator

martinheidegger commented Feb 7, 2018

Note: Its okay to force-push to a PR branch. 😉

@aks-
Copy link
Collaborator Author

aks- commented Feb 7, 2018

Right :D but I thought I would push it upstream and make a pr from there. Also I didn't know about the pattern we used for table row refactor, now I can use that to make this one more clearer.. and some relatively better naming. So i thought it would be okay to make entirely separate PR 🤔

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants