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

🎨 Add oss/cloud tags on doc for GA connectors #19118

Merged
merged 6 commits into from
Nov 17, 2022
Merged

Conversation

arnaudjnn
Copy link
Contributor

@arnaudjnn arnaudjnn commented Nov 8, 2022

What

Avoid unnecessary sections to improve the doc reading experience.

Fix titles hierarchy and naming for consistency.

How

Add the following tags on the sections dedicated to oss:

<!-- env:oss -->
<!-- /env:oss -->

Add the following tags on the sections dedicated to cloud:

<!-- env:cloud -->
<!-- /env:cloud -->

Preview

When you are in the Cloud version all the OSS paragraphs are hidden

Screenshot 2022-11-14 at 12 00 39

Screenshot 2022-11-14 at 12 00 29

@arnaudjnn arnaudjnn marked this pull request as ready for review November 8, 2022 11:41
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Nov 8, 2022
* Report Generation Maximum Retries
* Start Date (Optional)
* Profile IDs (Optional)
- Client ID
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a specific reason why we change the list syntax in all those files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is Prettier default conf. hyphen and asterisk are the same in markdown

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I know they are the same, but there is no setup that should run prettier on those files by default in the repository, so this might be a local setup in your IDE. We should in general not change a lot of unrelated syntax (like * into -) in a PR for something completely else. So We should make sure this PR is only touching adding the actual tags and changes related to it and not touching a lot of unrelated code in those files due to some local prettier setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you now what can I do to reverse the prettier changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure there's an easy way to do that, and since the "format" part is not in an isolated commit not sure reverting will help you.

What I would do:

  • double check your IDE conf to see what's causing this changes
  • maybe check how prettier is config on the project and match your IDE conf
  • reset your changes and remove all format related changes.
    I usually use git reset --soft commitHash to reset my changes in case it's helpful.
    https://git-scm.com/docs/git-reset

@arnaudjnn
Copy link
Contributor Author

@nataliekwong I handed over all the headers.

@arnaudjnn arnaudjnn requested a review from timroes November 10, 2022 12:07
Copy link
Contributor

@letiescanciano letiescanciano left a comment

Choose a reason for hiding this comment

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

I still see a lot of format/prettier changes included, making this PR having more changes that actually required.
Also, I see we are changing a lot of headers, example:
<img width="947" alt="Screen Shot 2022-11-11 at 9 04 27 AM" src="https://user-images.githubusercontent.com/45267095/201294231-0ee2a891-d954-4052-8b76-12c588c0b7d3.png”>

Is that intended?

My understanding (based on PR description) is that we just need to add <!-- env:oss —> or <!-- env:cloud —> tags to show specific docs depending on environment.

It would also be nice to have a small loom, where we can see an example of the changes

docs/integrations/sources/amazon-ads.md Show resolved Hide resolved
docs/integrations/sources/bing-ads.md Show resolved Hide resolved
docs/integrations/sources/file.md Show resolved Hide resolved
@arnaudjnn
Copy link
Contributor Author

I still see a lot of format/prettier changes included, making this PR having more changes that actually required. Also, I see we are changing a lot of headers, example: <img width="947" alt="Screen Shot 2022-11-11 at 9 04 27 AM" src="https://user-images.githubusercontent.com/45267095/201294231-0ee2a891-d954-4052-8b76-12c588c0b7d3.png”>

Is that intended?

My understanding (based on PR description) is that we just need to add <!-- env:oss —> or <!-- env:cloud —> tags to show specific docs depending on environment.

It would also be nice to have a small loom, where we can see an example of the changes

Can you comment on where there is this image changes?

As I have to edit all the GA connectors pages, I also fix some inconsistency + title errors that's it.

You can view Tim's demo here (12min): https://airbyte.zoom.us/rec/play/3vKBGVdKQw_EHFGFEEO6INqk7M6NwCTXyJTe-3hDdfgL7j6P3z5_ccgul1KAw3lki8opsg-IsXUxdmHl.a80AuC9qhhm8mHP7?continueMode=true&_x_zm_rtaid=9ZWepYheRw2rTR4gkxa3rA.1668421487226.86307acafb206ef549ab27dab128c422&_x_zm_rhtaid=241

@letiescanciano
Copy link
Contributor

I still see a lot of format/prettier changes included, making this PR having more changes that actually required. Also, I see we are changing a lot of headers, example: <img width="947" alt="Screen Shot 2022-11-11 at 9 04 27 AM" src="https://user-images.githubusercontent.com/45267095/201294231-0ee2a891-d954-4052-8b76-12c588c0b7d3.png”>
Is that intended?
My understanding (based on PR description) is that we just need to add <!-- env:oss —> or <!-- env:cloud —> tags to show specific docs depending on environment.
It would also be nice to have a small loom, where we can see an example of the changes

Can you comment on where there is this image changes?

As I have to edit all the GA connectors pages, I also fix some inconsistency + title errors that's it.

You can view Tim's demo here (12min): https://airbyte.zoom.us/rec/play/3vKBGVdKQw_EHFGFEEO6INqk7M6NwCTXyJTe-3hDdfgL7j6P3z5_ccgul1KAw3lki8opsg-IsXUxdmHl.a80AuC9qhhm8mHP7?continueMode=true&_x_zm_rtaid=9ZWepYheRw2rTR4gkxa3rA.1668421487226.86307acafb206ef549ab27dab128c422&_x_zm_rhtaid=241

Understood about the titles.
About the demo, I understand that these tags were created by the FE team and I saw there presentation. My ask is that we demo the changes INCLUDED in this PR, just a way to showcase an example of your work and provide context to the reviewer.

@arnaudjnn
Copy link
Contributor Author

@letiescanciano

I added 2 screenshots accordingly

@letiescanciano
Copy link
Contributor

@Amruta-Ranade this is ready for your review

Copy link
Contributor

@Amruta-Ranade Amruta-Ranade left a comment

Choose a reason for hiding this comment

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

Thanks for all the work, @arnaudjnn !!

Copy link
Collaborator

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Syntax looks good to me. Have not tested locally.

@arnaudjnn arnaudjnn merged commit 0164355 into master Nov 17, 2022
@arnaudjnn arnaudjnn deleted the oss-cloud-tags branch November 17, 2022 16:01
akashkulk pushed a commit that referenced this pull request Dec 2, 2022
* feat: add cloud and oss tags

* put headers back

* fix: rm prettier style

* fix: aws styles
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation team/growth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants