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

Create a levelbuilder-facing table of all code docs #45391

Merged
merged 6 commits into from
Mar 30, 2022

Conversation

bethanyaconnor
Copy link
Contributor

@bethanyaconnor bethanyaconnor commented Mar 18, 2022

Second to last PR for the all code docs page (well, until I have to update it for Java). The code change to add this page (even with two PRs already split out) is over 1000 lines so I decided to split out the component that loads a table of code docs (this PR) as it's more complicated than the others. Apologies that this is still a large PR on its own.

This component loads a table of code docs, optionally filtered using dropdown selections. I chose 20 entries per page a bit arbitrarily but it feels good for the page. These docs then have a button that opens a new tab to edit, a button to clone to another programming environment (dialog added in #45285). The routes to clone and destroy a programming expression were added in #45174.get_filtered_expressions, which gets the programming expressions according to the filters, is a new route added in this PR.

Changing environment and page:

Screen.Recording.2022-03-22.at.11.08.29.AM.mov

Changing category:

Screen.Recording.2022-03-22.at.11.08.55.AM.mov

Deleting a programming expression:

Screen.Recording.2022-03-22.at.11.09.16.AM.mov

Links

jira
product spec

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@bethanyaconnor bethanyaconnor marked this pull request as ready for review March 22, 2022 18:14
@bethanyaconnor bethanyaconnor requested review from a team March 22, 2022 18:15
Copy link
Contributor

@jamescodeorg jamescodeorg left a comment

Choose a reason for hiding this comment

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

Generally looks great! I found it a bit hard to parse the ProgrammingExpressionsTable component at first, maybe some extra comments or splitting up the main render method might help a bit?

(It kind of bugs me that our javascript style is so "vertical" but that's a rant for another day.)

Comment on lines 13 to 14
programmingEnvironmentsForSelect,
categoriesForSelect,
Copy link
Contributor

Choose a reason for hiding this comment

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

This might just be me, but I found these names a little confusing (since I didn't realize "select" referred to a select tag). I'm wondering if something like allProgrammingEnvironments and allCategories would be clearer? Up to you.

Copy link
Contributor

@jamescodeorg jamescodeorg left a comment

Choose a reason for hiding this comment

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

@jamescodeorg I tried using a reducer in df837a0 and I'm curious what you think. On one hand, it does move all the logic around the four entangled pieces of state into one place, which is nice. On the other hand, it didn't make sense to me to move all the state to the reducer so now the state is handled in two different ways. I could bring the rest of the state up into the reducer but that state is very simple.

I haven't used useReducer before and it doesn't look like it's been used in our codebase so I'm not sure what the conventions here are. It was easy to do (and a good leaning opportunity!) so if it isn't better, I'll just rebase it out.

Thanks for trying this out! Now that I see it, I think the previous code (not using useReducer) was a bit easier for me to follow but I am ok with either approach.

@bethanyaconnor
Copy link
Contributor Author

Thanks for trying this out! Now that I see it, I think the previous code (not using useReducer) was a bit easier for me to follow but I am ok with either approach.

I think I agree, plus I'd prefer to keep this file consistent so I'll take it out.

Copy link
Contributor

@jamescodeorg jamescodeorg 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 making all of the changes!

@bethanyaconnor bethanyaconnor merged commit 7225f6d into staging Mar 30, 2022
@bethanyaconnor bethanyaconnor deleted the bethany/all-code-docs-table branch March 30, 2022 19:22
@bethanyaconnor bethanyaconnor mentioned this pull request Apr 11, 2022
8 tasks
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