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

3324 retro review #3414

Merged
merged 2 commits into from
Mar 12, 2024
Merged

3324 retro review #3414

merged 2 commits into from
Mar 12, 2024

Conversation

christinaausley
Copy link
Contributor

Description

Retro review of #3324.

When should this change go live?

  • This change is not yet live and should not be merged until {ADD_DATE} (apply hold label or convert to draft PR)?
  • There is no urgency with this change.
  • This change or page is part of a marketing blog, conference talk, or something else on a schedule.
  • This functionality is already available but undocumented.
  • This is a bug fix or security concern.

PR Checklist

  • I have added changes to the relevant /versioned_docs directory, or they are not for an already released version.
  • I have added changes to the main /docs directory (aka /next/), or they are not for future versions.
  • My changes require an Engineering review, and I've assigned an engineering manager or tech lead as a reviewer, or my changes do not require an Engineering review.
  • My changes require a technical writer review, and I've assigned @christinaausley as a reviewer, or my changes do not require a technical writer review.

@christinaausley christinaausley added component:modeler Issues related with Modeler project component:docs Documentation improvements, including new or updated content labels Mar 5, 2024
@christinaausley christinaausley requested a review from toco-cam March 5, 2024 20:49
@christinaausley christinaausley self-assigned this Mar 5, 2024
@christinaausley christinaausley changed the title Docs 3324 retro 3324 retro review Mar 5, 2024
Copy link
Contributor

github-actions bot commented Mar 5, 2024

👋 🤖 🤔 Hello! Did you make your changes in all the right places?

These files were changed only in docs/. You might want to duplicate these changes in versioned_docs/version-8.4/.

  • docs/components/modeler/reference/modeling-guidance/rules/history-time-to-live.md

You may have done this intentionally, but we wanted to point it out in case you didn't. You can read more about the versioning within our docs in our documentation guidelines.

- Set a default HTTL via an engine configuration.
- Switch off the HTTL check via an engine configuration if history cleanup is not used.

## <MarkerGuideline.Info /> History time to live not configured

![History time to live not configured](./img/history-time-to-live/info.png)

In the screenshot above, note that the time to live must be defined under **History cleanup** in the properties panel.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For accessibility purposes and to explain the screenshot, I added the following sentence. WDYT @toco-cam ?

Copy link
Member

Choose a reason for hiding this comment

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

@christinaausley It is unfortunately not that easy, as there are three options to set the HTTL which are valid (Listed above the pic):

  • Define HTTL per model directly in Desktop Modeler.
  • Set a default HTTL via an engine configuration.
  • Switch off the HTTL check via an engine configuration if history cleanup is not used.

Copy link
Member

Choose a reason for hiding this comment

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

Who added the screenshot? The screenshot and your text only make sense for this bullet point "Define HTTL per model directly in Desktop Modeler."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @toco-cam -- this is actually based on your PR, if you'd like to make additional changes.

Copy link
Member

@toco-cam toco-cam Mar 21, 2024

Choose a reason for hiding this comment

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

Hi @christinaausley, ok, now it makes sense to me - all modeling guidance does have a screenshot displaying how the linting looks in the Modeler.

None of the other guidance has a caption - I would recommend not having one here, neither.

@christinaausley christinaausley requested a review from akeller March 11, 2024 14:55
Copy link
Member

@akeller akeller left a comment

Choose a reason for hiding this comment

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

I'm good with these changes

@christinaausley christinaausley merged commit 965cf2d into main Mar 12, 2024
11 checks passed
@christinaausley christinaausley deleted the docs-3324-retro branch March 12, 2024 15:49
@toco-cam
Copy link
Member

I'm good with these changes

Hey @christinaausley, did you consider my comment here before merging?

@christinaausley
Copy link
Contributor Author

@toco-cam This was merged two weeks ago, but I've kept this tab open to revisit, take a closer look, and add/remove captions as necessary based on your feedback.

theburi pushed a commit that referenced this pull request Apr 3, 2024
* style(formatting): technical review 3324

* clarify screenshot
theburi pushed a commit that referenced this pull request Jun 5, 2024
* style(formatting): technical review 3324

* clarify screenshot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:docs Documentation improvements, including new or updated content component:modeler Issues related with Modeler project
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants