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

Correct grammar, hyperlinks and formatting #17

Merged

Conversation

ipadurean-bd
Copy link
Contributor

@ipadurean-bd ipadurean-bd commented Feb 27, 2024

Correct acronym defined first for each document
Use Alliance instead of CSA (two terms: Alliance and CSA were used for the Connectivity Standard Alliance)
Correct grammar mistakes
Correct the hyperlinks
Correct formatting
Apply uniform formatting of lists with dot at the end

@CLAassistant
Copy link

CLAassistant commented Feb 27, 2024

CLA assistant check
All committers have signed the CLA.

@sammachin
Copy link
Contributor

@ipadurean-bd thanks for the contribution,

We don't use the acronym CSA we prefer "The Alliance" as a shortened term.
Other fixes look good, could you update your PR to remove the CSA changes

@ipadurean-bd
Copy link
Contributor Author

@sammachin thank you!
I updated the PR so it contains the preferred shortened term

@ipadurean-bd ipadurean-bd force-pushed the ipadurean-corrections-bitdefender branch 2 times, most recently from f201639 to 69427cf Compare March 1, 2024 07:17
@ipadurean-bd
Copy link
Contributor Author

@sammachin I see the Deploy PR previews keep failing. Is there anything I should change to the PR?

@sammachin
Copy link
Contributor

@ipadurean-bd yeah I thought I'd fixed that yesterday, but your's is the first PR using it that's coming from an external repo so I can see its still failing. I'll take a look at that today,

@sammachin
Copy link
Contributor

@ipadurean-bd
I think I've found the issue, could you fetch the latest changes into your fork and it should then run, it was using the variables and secrets from your fork rather than the main repo

@ipadurean-bd ipadurean-bd force-pushed the ipadurean-corrections-bitdefender branch from 69427cf to c547ebd Compare March 1, 2024 08:41
Copy link

github-actions bot commented Mar 1, 2024

Preview deployed at https://d1teub3jrrfss6.cloudfront.net/pr-17

@ipadurean-bd
Copy link
Contributor Author

@sammachin done

@sammachin
Copy link
Contributor

seems like the action was still checking out the main branch not your head, I've just updated it so could you fetch that change and we'll see what it builds this time

Copy link

github-actions bot commented Mar 1, 2024

Preview deployed at https://d1teub3jrrfss6.cloudfront.net/pr-17

@ipadurean-bd
Copy link
Contributor Author

@sammachin what is the situation on this pull request? It cannot be merged?

@sammachin
Copy link
Contributor

@ipadurean-bd could you pull in the latest changes from main, that should trigger the preview to be re-deployed, there were some bugs in the github action used for the previews but it was also running the action in main and showing that as the preview without your changes!
I also commented above that the change to /static for images is wrong, the handbook isn't meant to be read from github this is just the source for the static site generated with hugo and published at https://handbook.buildwithmatter.com

@ipadurean-bd ipadurean-bd force-pushed the ipadurean-corrections-bitdefender branch from c547ebd to 5b214c5 Compare March 13, 2024 09:33
Copy link

Preview deployed at https://d1teub3jrrfss6.cloudfront.net/pr-17

@ipadurean-bd
Copy link
Contributor Author

@sammachin so I should revert the images changes?

@sammachin
Copy link
Contributor

@sammachin so I should revert the images changes?

Yes please, as you can now see in the preview the images don't load https://d1teub3jrrfss6.cloudfront.net/pr-17/howitworks/datamodel/

Copy link

Preview deployed at https://d1teub3jrrfss6.cloudfront.net/pr-17

Correct acronym defined first for each document
Use Alliance instead of CSA (two terms: Alliance and CSA were
used for the Connectivity Standard Alliance)
Correct grammar mistakes
Correct formatting
Apply uniform formatting of lists with dot at the end
@ipadurean-bd ipadurean-bd force-pushed the ipadurean-corrections-bitdefender branch from 4821db3 to 836e8fa Compare March 13, 2024 09:59
Copy link

Preview deployed at https://d1teub3jrrfss6.cloudfront.net/pr-17

@ipadurean-bd
Copy link
Contributor Author

@sammachin I reverted the hyperlinks, but the preview still doesn't load the images

@sammachin
Copy link
Contributor

@sammachin I reverted the hyperlinks, but the preview still doesn't load the images

ah I think that might be the cloudfront preview caching the previous pages, 2 mins I'll take a look

@sammachin
Copy link
Contributor

yes it was the caching, I'll need to add an invalidation to the action in future,

Thanks I'll review the rest of the edits today

Copy link
Contributor

@sammachin sammachin left a comment

Choose a reason for hiding this comment

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

One small edit but otherwise looks good

content/HowItWorks/Commisioning.md Outdated Show resolved Hide resolved
@@ -5,12 +5,10 @@ weight = 2
pre = "<b>2. </b>"
+++


# The Connectivity Standards Alliance
# The Connectivity Standards Alliance (The Alliance)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# The Connectivity Standards Alliance (The Alliance)
# The Connectivity Standards Alliance

I agree that its useful to explain acroynms but this makes the H1 on the page far to long

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link

Preview deployed at https://d1teub3jrrfss6.cloudfront.net/pr-17

@sammachin sammachin merged commit 67cc4c7 into project-chip:main Mar 14, 2024
3 checks passed
@sammachin
Copy link
Contributor

thanks all looks good/

Copy link

Preview deployed at https://d1teub3jrrfss6.cloudfront.net/pr-17

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.

3 participants