-
-
Notifications
You must be signed in to change notification settings - Fork 148
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
Add capture as a new header flag #936
base: master
Are you sure you want to change the base?
Conversation
Hi @iocanel Sounds like a great idea! I tried it out, but couldn't really get it to work as expected. Maybe I'm using it wrong. I tried adding a new line a table. Given this header with a table (taken from sample.org)
and this capture template: Then, when I want to add something to the table for the first time, I get this result:
There's one newline too many, so it's two tables now. When I call the capture template one more time (a second time), this is the result:
So the table row from the second call to the capture template is added to the right of the row from the first call to the capture template. In summary, both the first and the second call to the capture template did not append to a table. Here's how it looks in the GUI to me: |
Apart from these two findings, I like the general idea! I'm not a hundred on whether or now I want to see the two boolean flags every time I use a capture templates, but maybe it's a good thing. I'll sleep on it(; |
Let me change the default value, so that existing templates don't break and see what I can do with the newline handling. |
I updated the PR with two new commits that handle spaces and the absence of |
7f94cbc
to
54cb650
Compare
Hi @iocanel Thank you for taking another look at this! I just tested again by pulling your branch and restarting the server. Unfortunately, existing capture templates aren't treated as 'capture as new header'. I made a video to demonstrate: https://www.dropbox.com/s/s120b3q8vu19dhd/2023-01-28%2011-35-56.mkv?dl=0 Unfortunately, this would break existing capture templates, so i cannot merge at this time. |
@munen: Thanks for the detailed feedback. Let me have a look. |
Apart from that, I'd like to discuss your choice to add "Should capture as new header" to the When it comes to 'should capture as new header', I cannot really think of such a template. If it's a "TODO" header, it's always a new header. If it's inline content, like a new table row, it never is. Did you have a scenario in mind where you need to switch the flag on the fly? |
Wow, just 5 minutes response time, even though it took me a while to get back to your changes. You're a 🚀, @iocanel 💯 (; |
I just happened to be around. 😄 |
BTW, I am not able to reproduce what I am seeing on the video, which makes me wonder if the template was Give me a sec, to share what I experience myself. |
Ok, it seems that I was the one with the |
When I tested, I thought the same. So I checked my |
hihi, it happens to the best 😄 |
Pushed a fix. Just to be sure I switched to the master branch and recreated my templates from scratch. So, hopefully it should work fine. Regarding the model window, if felt that it rhymes with the prepend, but I don't have a use case forcing it. So, I don't mind removing it. |
@iocanel Sweet! When using my old capture templates, they do show 'create new header', now! However, your fix is 'only' inline the capture template modal. Other than patching new functionality inline, I think a more solid approach is to write a migration (https://github.com/200ok-ch/organice/tree/master/src/migrations), so that all existing capture templates will actually have the new flag set to Also, I've tested adding things to header descriptions like tables and it works a treat. I did find a bug concerning tables, but we can totally leave it if you think it's an edge case: https://www.dropbox.com/s/cnjrlqq37ib506m/2023-01-28%2013-01-44.mkv?dl=0
If you only added it for consistency, but also don't see a personal reason to use it, I'd say thank you for taking the extra effort to make it consistent, but let's remove the feature that we both see no reason to use(; |
When these things are done, the last part would be to get the tests to be green, again. |
Ok, I'll need to check how migrations are used. |
I am tying something like: import { localStorageAvailable } from '../util/settings_persister';
export default () => {
if (!localStorageAvailable) {
return;
}
let captureTemplates = JSON.parse(localStorage.getItem('captureTemplates'));
captureTemplates = captureTemplates || {};
captureTemplates.forEach(t => {
if (!t.hasOwnProperty('shouldCaptureAsNewHeader')) {
t.shouldCaptureAsNewHeader = true
}
});
localStorage.setItem('captureTemplates', JSON.stringify(captureTemplates));
}; Even though I updated the PR, but it's not workign as expected. Will have another look later. Meanwhile if there is soemthing obviously missing, please let me know. |
Resolves: #934
The pull request introduces an option in the capture template definition and capture modal that allows the capture content to be added as a new header or appended / prepended to the existing content. Practially this allows capturing table rows (even though the solution is more generic).