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

Blockly: minor restructure and grammatical improvements #2263

Closed
wants to merge 2 commits into from

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Feb 29, 2024

@stefan-hoehn I've been working on this.

It is based on #2262, but the main changes are on the following pages (for now)

  • blockly/index.md
  • blockly/rules-blockly-getting-started.md
  • blockly/rules-blockly-date-handling.md

Please ignore the changes in all the other files for now. They are minor changes (blockly -> Blockly). The three files above are where my main changes were made.

I am glad you created the documentation in the first place and I appreciate your hard work.

What do you think of the changes? If you're happy with the idea / direction, I will continue working on the rest of the docs. It is a very time consuming work so I'd like to get your feedback first before proceeding any further.

It would be great if we had a way to generate the docs for preview purposes.

@jimtng jimtng force-pushed the blockly-edit branch 2 times, most recently from b8bf2bd to 0fb491a Compare February 29, 2024 14:19
@stefan-hoehn
Copy link
Contributor

stefan-hoehn commented Mar 2, 2024

Thanks, Jim, as always for putting so much work into it. 🥳
So nothing below is meant in an offensive way 🙏🏻 which is even why I a bit procrastinated giving this feedback 😱

In general, the more you change the more time it takes me to review it to make sure nothing gets lost or is changed in a way I wouldn't be happy of
.
I am a bit unclear how to proceed because it is extremely hard to understand what really has changed.

  • As you have marked it as draft, I guess (and I should know this), this hasn't even been built by netlify, so there is no easy way to see the changes as a preview (I have now checked it out locally, though, so I can review it better)
  • Only after some time I understood (correct my if I am wrong) that you renamed rules-blockly-before-using.md into rules-blockly-getting-started.md. Unfortunately, this has a major drawback because I can't review anymore what has changed. Hence, I would prefer first only to change the content and then later after a first review provide a 2nd commit that does the rename. So, would you mind to revert the rename(s) first to allow easier review for me? (only for those pages, if there are more, where the problem applies)
  • Secondly, I noticed that quite a number of images were deleted which kind of worries me because I don't understand, why.
  • Third: You did so many changes in the index.md that I fear that much of it has been thrown away and might have gotten lost. But even after looking at it I can hardly recognize the changes. So I'll have a hard time to find out what really changed. (btw, it now in the meantime has conflicts with a different change surpassing this PR)

In general, my recommendation is: if you like to provide a PR and it is a major change or contribution it is better (at least) for me if you propose something first because that can avoid frustration later on if it would be questioned by someone (like me ;-)) which is something I totally like to avoid as I am grateful for many of the contributions that come in as PRs like you! 😇

Again, don't get me wrong: you put quite some time into that but it also means I need to invest a lot of time for the review. I surely don't just wave it through, so I am kind of desperate how to move on. What I can I offer is to have a remote session together where we go through it together - maybe that is more efficient. ... and my goal is to be as quick as possible with reviews and keep the PR list very short 🥸

Many other changes that I saw are mostly english language improvements which I (not being a native speaker) of course very much appreciate

cheers,
Stefan

@jimtng
Copy link
Contributor Author

jimtng commented Mar 2, 2024

Hi Stefan,

Thanks for looking into this.

I'll try to explain a bit further here:

  • In index.md, I removed all the extra explanations + small image preview for each of the sub sections. The goal is to make it easier to see the actual list of subsections and read only a brief description of it in one sentence. Currently in my opinion the additional text + small images only distract readers from getting a general overview of the sections.

When they want to learn about any particular section, they can click on to it to then view the details. This separates the function of index.md vs the detailed sub-pages.

Also several of the small images that I've removed are not readable anyway because they're too small. My eyes are not as sharp as they used to these days. Unfortunate effect of aging :(

My suggestion is not to try to compare it by looking at the DIFF, but instead, view the old page and the new page side by side, as rendered / previewed pages.

It is impossible to do a makeover by only changing punctuations here and there. Sometimes the whole structure needs to be completely changed, so looking at diff wouldn't give you an understanding of the changes.

What I can I offer is to have a remote session together where we go through it together

Do you have a medium to do this? I'm more comfortable with text based chats, e.g. Slack / Discord / Whatsapp.

my goal is to be as quick as possible with reviews and keep the PR list very short

You're doing a great job at keeping the PRs under control!

This particular PR though, if I were to proceed with the rest, is going to take some time, several days / weeks. That's why I wanted to ask your feedback first before spending more time.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 2, 2024

I should also add.... I prefer succinct, straight to the point type of documentation. This is because it makes it so much quicker to get to the information. This is why I removed a lot of the "introduction" type paragraphs that doesn't offer any actual information about Blockly and how it works.

In my opinion, the current openHAB documentation suffers from this issue overall, not just within the blockly section.

Also some parts were moved around, e.g. the migration from oh3 to oh4 was moved towards the end of the document.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 2, 2024

So nothing below is meant in an offensive way 🙏🏻

I also hope that none of what I have done / written is perceived as offensive. At the end of the day, I just want to help make the best possible openhab. I understand that disagreements may arise.

@jimtng jimtng closed this Mar 2, 2024
@jimtng jimtng reopened this Mar 2, 2024
@jimtng
Copy link
Contributor Author

jimtng commented Mar 2, 2024

sorry clicked the wrong button

@jimtng
Copy link
Contributor Author

jimtng commented Mar 2, 2024

I have renamed the file back to rules-blockly-before-using.md

@jimtng
Copy link
Contributor Author

jimtng commented Mar 3, 2024

  • As you have marked it as draft, I guess (and I should know this), this hasn't even been built by netlify, so there is no easy way to see the changes as a preview (I have now checked it out locally, though, so I can review it better)

Would you like me to mark it as Ready for review, so it will be built by netlify?

I've been meaning to look into the local build process one of these days.

@jimtng jimtng changed the title Blockly edit [WIP] Blockly edit Mar 3, 2024
@jimtng jimtng marked this pull request as ready for review March 3, 2024 02:55
@jimtng
Copy link
Contributor Author

jimtng commented Mar 3, 2024

Is Netlify disabled / broken?

I found #2218 and ran the preview locally. Nice!!

@stefan-hoehn
Copy link
Contributor

I should also add.... I prefer succinct, straight to the point type of documentation. This is because it makes it so much quicker to get to the information. This is why I removed a lot of the "introduction" type paragraphs that doesn't offer any actual information about Blockly and how it works.

In my opinion, the current openHAB documentation suffers from this issue overall, not just within the blockly section.

I disagree at that point, Jim and I will not remove this introduction part.

@stefan-hoehn
Copy link
Contributor

Is Netlify disabled / broken?

Why do you think that Netflify is broken, can you give me an example?

@jimtng
Copy link
Contributor Author

jimtng commented Mar 3, 2024

I disagree at that point, Jim and I will not remove this introduction part.

Which part specifically that you wish to keep?

Is Netlify disabled / broken?

Why do you think that Netflify is broken, can you give me an example?

Usually there's a special post made by Netlify, but that doesn't exist here:
#2262
#2257

@stefan-hoehn
Copy link
Contributor

You don't have to mark it as Review to run the preview because I can review it locally. However, I won't be able to work on that before long because the changes you did are too comprehensive and differ too much from what I originally wrote. Please rather provide a PR that corrects documentation than rather delete parts that you think should be taken out.
In the end this all about personal taste of documentation unless it is wrong or completely superfluous where we can always restructure text in sub pages.

If the latter was the case open an issue before and discuss the fact first and propose changes and keep topics and changes small.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 3, 2024

So to summarise, this PR rejected because it changes the structure too much from the original documentation? Once the documentation is written, it's set in stone and only minor typos and additions are accepted?

I'm fine with that, as long as I understand the policy, so I won't worry on suggesting any structural changes in the future.

If the latter was the case open an issue before and discuss the fact first and propose changes and keep topics and changes small.

I feel that making a PR to clearly show you exactly what I'm proposing is easier for you to see/preview rather than just describing it. Feel free to close this PR if the idea is completely rejected.

Alternatively it can be adjusted as a normal PR review process, so we're all happy with the end result.

As the custodian of the documentation, it's up to you. I can only voice my suggestions.

@stefan-hoehn
Copy link
Contributor

So to summarise, this PR rejected because it changes the structure too much from the original documentation? Once the documentation is written, it's set in stone and only minor typos and additions are accepted?

This is not what I said, Jim, and you surely know this. This documentation is not set in stone once it is written but similar to code when you want to restructure and propose a major change (like removing functionality) I asked you to propose that change before you provide a comprehensive PR like this one.

As I mentioned before I don't agree that we should throw away the introduction information like you proposed. You, as a proficient person, may not need that but others appreciate it.

There are improvements in this PR that I would agree to, others I am not happy with.

Honestly I don't know how to accept part of the changes and others not.

Maybe we can ask others what they think, for example @Confectrician , @rkoshak, @florian-h05 and @Larsen-Locke to provide their opinion what they think about removing/restructuring what you proposed.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 3, 2024

This is not what I said, Jim, and you surely know this.

No, I did not know this. Perhaps I don't really understand what you're saying. There seems to be a miscommunication somewhere. I have stated my understanding upon reading your response, and at the moment I don't know how to proceed. I feel like you do not want any changes in the structure of the documentation, and only willing to accept sentence by sentence changes. That's how I understand it. This is perfectly fine of course, and that's why I haven't done all the changes I had in mind and only worked on 3 files, so if this isn't the direction you'd like, I won't need to continue.

I am also under the impression that I would need to submit individual PRs for each small change needed, which is going to be quite difficult to coordinate when the scope of the changes are bigger, some involving moving sections from one file/page to another.

As I see it, there are several options:

  • Continue and make adjustments based on your/others' feedback.
  • Abandon this PR, and perhaps in separate PRs make micro changes here and there fixing minor typos only.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 3, 2024

Honestly I don't know how to accept part of the changes and others not.

As I understand it, you would tell me what you want changed, I'll make the changes, and push more commits for you to review again.

@jimtng jimtng changed the title [WIP] Blockly edit Blockly: minor restructure and grammatical improvements Mar 4, 2024
@jimtng jimtng marked this pull request as draft March 4, 2024 10:52
Copy link

netlify bot commented Mar 4, 2024

Thanks for your pull request to the openHAB documentation! The result can be previewed at the URL below (this comment and the preview will be updated if you add more commits).

Name Link
🔨 Latest commit 55ab2af
🔍 Latest deploy log https://app.netlify.com/sites/openhab-docs-preview/deploys/6612efe3025708000894aa9f
😎 Deploy Preview https://deploy-preview-2263--openhab-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jimtng jimtng closed this Apr 19, 2024
@jimtng jimtng deleted the blockly-edit branch April 20, 2024 14:50
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