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

Modernize Process States Overview #23

Merged
merged 6 commits into from Jul 11, 2023

Conversation

dviererbe
Copy link
Contributor

@dviererbe dviererbe commented May 30, 2023

I found out that GitHub supports mermaid (basically a code-to-diagram framework). I also overhauled the text content/arrangement in the section a little bit.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@dviererbe
Copy link
Contributor Author

note: it is fine, that the spell checker fails. #20 fixes the errors.

@dviererbe dviererbe force-pushed the modernize-process-states-overview branch from daabd4d to 5d7a6c9 Compare May 31, 2023 06:41
@github-actions

This comment has been minimized.

Copy link
Collaborator

@cpaelzer cpaelzer left a comment

Choose a reason for hiding this comment

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

This is a great start, I've left a few suggestions as well as questions to make even more use of the new ability to be less space limited now.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
%% mermaid flowcharts documentation: https://mermaid.js.org/syntax/flowchart.html
%%{ init: { 'flowchart': { 'curve': 'monotoneY' } } }%%
flowchart TD
subgraph New["New / Confirmed¹"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

An important aspect of the old graph was that each valid state (anything not in the graph is invalid) has a tuple of Assignment+State. This is somewhat lost now. A few examples:

This is great, but currently I miss a bit of continuity.
At the top the state set for new/confirmed is a yellowish box.
Down there all the state set actions for "Won't Fix", "In Progress" are purple boxes.
But at the top purple boxes are not state changes, they are assignee changes.

Could we make all state-changes purple boxes, and make all assignment changes e.g. yellow hexagons or something like that. So that the shape/color directly indicates what is expected to happen at this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although mermaid flowchart's allow to set the color, I would recommend against it, because if we set a color, it does not change with the github theme (selected by a user).

This can result in bad visibility for people that require e.g. a dark high contrast theme.

But I incorporated more distinguishable shapes and removed the subgraph.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link

@check-spelling-bot Report

🔴 Please review

See the 📂 files view or the 📜action log for details.

Unrecognized words (2)

URLs
workaround

To accept ✔️ these unrecognized words as correct, run the following commands

... in a clone of the git@github.com:dviererbe/ubuntu-mir.git repository
on the modernize-process-states-overview branch (ℹ️ how do I use this?):

curl -s -S -L 'https://raw.githubusercontent.com/check-spelling/check-spelling/main/apply.pl' |
perl - 'https://github.com/canonical/ubuntu-mir/actions/runs/5256386511/attempts/1'
Available 📚 dictionaries could cover words not in the 📘 dictionary

This includes both expected items (10) from .github/actions/spelling/expect.txt and unrecognized words (2)

Dictionary Entries Covers
cspell:cpp/src/cpp.txt 30216 4
cspell:win32/src/win32.txt 53509 2
cspell:php/php.txt 2597 2
cspell:cpp/src/stdlib-c.txt 290 2
cspell:ruby/ruby.txt 278 1
cspell:r/src/r.txt 808 1
cspell:python/src/python/python-lib.txt 3873 1
cspell:fullstack/fullstack.txt 390 1

Consider adding them using (in .github/workflows/spelling.yml):

      with:
        extra_dictionaries:
          cspell:cpp/src/cpp.txt
          cspell:win32/src/win32.txt
          cspell:php/php.txt
          cspell:cpp/src/stdlib-c.txt
          cspell:ruby/ruby.txt
          cspell:r/src/r.txt
          cspell:python/src/python/python-lib.txt
          cspell:fullstack/fullstack.txt

To stop checking additional dictionaries, add:

      with:
        check_extra_dictionaries: ''
✏️ Contributor please read this By default the suggested command will add the listed items to the .github/actions/spelling/expect.txt. This is not allways desired!

If a listed items is

  • ... misspelled, then please correct them instead of changing the spell checker configuration.
  • ... an actual word/term that has a high probability of showing up in future contributions, please add it to the allowed words.
  • ... an term/word that just you use or shouldn't generally be accepted, please add it to .github/actions/spelling/expect.txt.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push – it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

@dviererbe dviererbe requested a review from cpaelzer June 27, 2023 12:20
Copy link
Collaborator

@cpaelzer cpaelzer left a comment

Choose a reason for hiding this comment

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

Oh yeah, this looks much nicer.
I can see states, differentiate them from final-states, triggers, actions/decisions.

Let us bring the pre-rendering vs old ascii-art up in next weeks meeting for group exposure before merging.

@cpaelzer
Copy link
Collaborator

cpaelzer commented Jul 7, 2023

@cpaelzer cpaelzer merged commit ee3325b into canonical:main Jul 11, 2023
3 of 4 checks passed
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.

None yet

2 participants