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

DOC/MAINT: add documentation, docking system conda shim #128

Merged
merged 13 commits into from
Dec 14, 2023

Conversation

ZLLentz
Copy link
Member

@ZLLentz ZLLentz commented Dec 6, 2023

closes #125

I intended to test this offline but couldn't quickly get the docs building on my local windows machine, so I'm going to iterate by downloading the CI docs build generated from this PR and checking that it looks normal.

  • Document how to configure the toolbar
  • Document how to configure the grid
  • Fix broken conda CI (PyQtAds submodule import name changed, again)

@ZLLentz ZLLentz changed the title WIP: add documentation WIP: add documentation, docking system conda shim Dec 6, 2023
@ZLLentz ZLLentz changed the title WIP: add documentation, docking system conda shim DOC/MAINT: add documentation, docking system conda shim Dec 6, 2023
@ZLLentz ZLLentz marked this pull request as ready for review December 6, 2023 19:47
@ZLLentz
Copy link
Member Author

ZLLentz commented Dec 6, 2023

I'm now fairly confident this is correct and useful. In about 5 mins the GHA will build the docs again which is the preferred way to review the html.

@ZLLentz ZLLentz requested a review from tangkong December 6, 2023 19:48
tangkong
tangkong previously approved these changes Dec 6, 2023
Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

I like the docs. I do think pictures would be immensely helpful but if we're going to be revamping the config system soon it's probably fine to not polish this too thoroughly.

A couple of thoughts I had while reading through the docs, I do feel like these are even more optional suggestions than normal.

LUCID Grid Configuration
========================

``LUCID``'s main grid area is an auto-generated series of buttons and indicators,
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
``LUCID``'s main grid area is an auto-generated series of buttons and indicators,
``lucid``'s main grid area is an auto-generated series of buttons and indicators,

Should we keep this consistent throughout the docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm trying to use LUCID when talking about the app and lucid when talking about the cli invocation, but:

  • I may not have done this correctly in general
  • This might not be the right approach

I'm tempted to turn it all into lucid in general and Lucid in titles

Copy link
Member Author

Choose a reason for hiding this comment

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

A lot of this stems from the readme and docs previously using LUCID exclusively despite the lowercase cli entrypoint + my dislike of all caps or special caps names in general

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like my opinion on the matter changes daily. I think it's fine as is, it just stuck out as the only fully-capitalized instance on the page

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to go ahead and change it to my preferences of Lucid or lucid everywhere (down with arbitrary casing!). I think this is noncontroversial because Lucid is essentially a normal word despite being an acronym.

docs/source/toolbar.rst Outdated Show resolved Hide resolved
docs/source/toolbar.rst Outdated Show resolved Hide resolved
@ZLLentz
Copy link
Member Author

ZLLentz commented Dec 6, 2023

RE: pictures:

  • I need to learn how to include these (kind of silly that I don't know)
  • The control system isn't up right now to take pictures

But I think these are both worth doing

tangkong
tangkong previously approved these changes Dec 6, 2023
Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

LGTM

@ZLLentz
Copy link
Member Author

ZLLentz commented Dec 6, 2023

I found an image of lucid on confluence, so I'm going to edit it a bit and use it here to have one simple visual on each of these two pages

@ZLLentz
Copy link
Member Author

ZLLentz commented Dec 6, 2023

snapshot of how the two new pages look with the images:
image
image

- ``functional_group``, to select with row to put a device into

That is to say, by default we expect the row headers on the left to be functional names like
"imager" or "vacuum" and column headers on the top to be location names like "SB3" or "VLS".
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
"imager" or "vacuum" and column headers on the top to be location names like "SB3" or "VLS".
"imager" or "vacuum" and column headers on the top to be location names like "Section 01" or "Section 02".

I like the docs a lot better with pictures, one last nitpick might be to make the examples match the images.

We could take this a step further with the buttons config, but if we do I'd advocate showing two configs. One with the descriptive values and one that matches the toolbar shown in the image.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'm not motivated to make an example config that matches the image, I'd be better off redoing the images to be the current config and pasting the current KFE config in.

Instead, I'll add a link to the lucid configs backup repo (which I'll update again at some point) so at least there's a way to see a bunch of valid configs.

I'm also going to merge your suggestion here

Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

LGTM

@ZLLentz ZLLentz merged commit 6b96a04 into pcdshub:master Dec 14, 2023
9 checks passed
@ZLLentz ZLLentz deleted the doc_toolbar_file branch December 14, 2023 00:42
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.

Write documentation for lucid
2 participants