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

Set up foundation for translations page #107

Merged
merged 46 commits into from
Jul 14, 2024

Conversation

henrycatalinismith
Copy link
Collaborator

demo.mp4

There's a few things going on here so here's a quick executive summary.

  1. I'm bringing in some layout work inspired by the "Order Dashboard" Joy UI template. It fits really nicely with the sidebar/main layout in ✏️ Translation page #73, and saves a fair bit of work.
  2. The message tree for Message list #83 is powered by @mui/x-tree-view/RichTreeView. This is a really mature & flexible component that handles stuff like keyboard interactions for free.
  3. That RichTreeView component didn't work at all with @mui/joy. I tried several times. Eventually what got it working was to replace @mui/joy with @mui/material.
  4. The sidebar & messages are organised using Next's parallel routes functionality. We did a big comparison of routing options and this was the one that satisfied all the constraints the best.
  5. Translations are saved using a Next.js server action.

This is unfinished work. I don't normally like to send a pull request this big, and would have split this up into smaller pieces if I'd been able to find a way to split it into cohesive parts. Here's a sample of some things that are either missing, broken, or that I'm otherwise unsatisfied with.

  • I didn't look at getting the "Send pull request button" working again yet.
  • I'm convinced the fine details of how the tree behaves will be subject to considerable tweaking and I want that tweaking to be mostly driven by feedback from real use.
  • The migration from @mui/joy to @mui/material has uglified the theme a little bit, which needs some attention later.
  • I've not really tweaked the MessageForm component here except for updating its state handling, and it needs a bit of visual work to avoid stuff like message IDs overflowing and obscuring the textarea.

Thing is, this is actually a complex enough little app that it needed a fair bit of structure adding just to get it into a place where those kinds of individual problems become addressable on an individual basis. So it felt necessary to just do one big first cut PR to get that structure in place, and that's what this is.

yarn.lock Outdated Show resolved Hide resolved
@WULCAN WULCAN linked an issue Jul 5, 2024 that may be closed by this pull request
7 tasks
@WULCAN WULCAN mentioned this pull request Jul 5, 2024
Copy link
Collaborator

@WULCAN WULCAN left a comment

Choose a reason for hiding this comment

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

Thank you for this pull request. I learned a lot by reviewing it and it adds a high value feature: the navigation tree of #73

  • There's an important regression about saving translations that I think we should fix before merging, see comment below.
  • I think we can implement the parallel loading of the sidebar better with <Suspense> in the layout, see comment below. Since that decision is going to have a large effect on coming changes, I think we should at least discuss it before merging this solution.
  • There are a couple of minor things too, see the comments.
  • Last but not fewest, I left a bunch of notes which might or might not interest you.

webapp/src/components/MessageForm.tsx Outdated Show resolved Hide resolved
webapp/src/components/MessageForm.tsx Outdated Show resolved Hide resolved
webapp/package.json Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
Copy link
Collaborator

@WULCAN WULCAN 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 sorry about dragging this out so much. We're very close to merging now.

package.json Outdated Show resolved Hide resolved
webapp/src/components/MessageList.tsx Outdated Show resolved Hide resolved
webapp/src/components/MessageForm.tsx Show resolved Hide resolved
webapp/src/components/MessageList.tsx Outdated Show resolved Hide resolved
webapp/src/components/MessageList.tsx Outdated Show resolved Hide resolved
webapp/src/components/MessageForm.tsx Outdated Show resolved Hide resolved
henrycatalinismith and others added 3 commits July 14, 2024 08:52
Co-authored-by: WULCAN <7813515+WULCAN@users.noreply.github.com>
@henrycatalinismith
Copy link
Collaborator Author

henrycatalinismith commented Jul 14, 2024

Unable to reproduce the issue of the list being scrollable while the sidebar is open.

Do I need to try this on a specific browser & OS or something?

@henrycatalinismith
Copy link
Collaborator Author

Used the same "Make macOS scroll bars work like on Linux & Windows" trick as in zetkin/app.zetkin.org#1963 to reproduce the double scrollbar issue and fixed it in 0cd0051

@WULCAN
Copy link
Collaborator

WULCAN commented Jul 14, 2024

Unable to reproduce the issue of the list being scrollable while the sidebar is open.

Do I need to try this on a specific browser & OS or something?

I can scroll it by switching focus with the tab key on laptop Chromium. It's not a problem. I'm just not certain what our intention was with toggling overflow: hidden on body.

WULCAN
WULCAN previously approved these changes Jul 14, 2024
Copy link
Collaborator

@WULCAN WULCAN left a comment

Choose a reason for hiding this comment

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

Great! I think there's a small small problem with overflow, see comment below. I think we are ready to merge this.

webapp/src/app/layout.tsx Outdated Show resolved Hide resolved
webapp/src/components/MessageList.tsx Outdated Show resolved Hide resolved
webapp/src/components/MessageList.tsx Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@henrycatalinismith
Copy link
Collaborator Author

henrycatalinismith commented Jul 14, 2024

Nice catch with the keyboard navigation in the mobile layout. Reminded me to add a <FocusTrap> around the sidebar contents when open on mobile (4dfeebb). Nice to be rid of that ugly utils/sidebar.ts thing as well. Also pushed a8da526 so that the overflow: hidden stuff doesn't affect other pages while this one is in such a half-finished state.

Copy link
Collaborator

@WULCAN WULCAN left a comment

Choose a reason for hiding this comment

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

Thank you! I can't help but find new small problems, see below, but they are not serious enough to block merge.

webapp/src/components/MessageList.tsx Show resolved Hide resolved
webapp/src/components/Sidebar.tsx Show resolved Hide resolved
@henrycatalinismith henrycatalinismith merged commit a39fc65 into main Jul 14, 2024
1 check passed
@henrycatalinismith henrycatalinismith deleted the issue-73/translation-page branch July 14, 2024 15:06
This was referenced Jul 14, 2024
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.

✏️ Translation page
4 participants