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

Add the components package with PathMappingControl #1608

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

adamziel
Copy link
Collaborator

Motivation for the change, related issues

Adds a PathMappingControl React component:

CleanShot 2024-07-12 at 15 11 01@2x

The short-term goal is to eventually replace the current mapping control based on text inputs:

CleanShot 2024-07-12 at 15 13 00@2x

The long-term goal is to create a library of dependency-free components (well, aside of React) to reuse between Playground webapp, WordPress plugins, WordPress blocks, and the Blueprints builder. This path mapping widget will make a good fit for all these places.

Implementation details

The PathMappingControl uses WordPress components under the hood. Most notably, TreeGrid and Button. I considered using web components for portability, but decided against it because they:

  • Wouldn't have the native WordPress look and feel
  • Couldn't easily mix with WordPress components
  • Had some issues around focus management

Testing Instructions (or ideally a Blueprint)

  1. Run nx dev playground-components
  2. Go to http://localhost:5173/
  3. Play with the widget and confirm it works intuitively

Follow-up work

  • Replace the text input under Absolute path in Playground with a UI-based picker or path autocompletion.
  • Implement mapping validation to make sure there are no nested mappings, e.g. /wordpress/wp-content and /wordpress/wp-content/plugins.
  • Reuse the base TreeGrid as a file browser in the Playground block (need asynchronous loading)
packages/playground/components/vite.config.ts Outdated Show resolved Hide resolved
};

const handlePathChange = (value: string) => {
updateNodeState(path, { playgroundPath: value });
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should check if the path is valid. For example, I'm able to store a space as a path.

Screenshot 2024-07-15 at 07 24 10

Copy link
Collaborator Author

@adamziel adamziel Jul 15, 2024

Choose a reason for hiding this comment

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

Agreed on validation, although a space is a valid path:

$ echo "test" > " "
$ cat " "
test

In fact, almost everything is a valid path. There are some characters that can't be used in paths, such as :. AFAIR paths are bytes and don't conform to any specific encoding like UTF-8. More research is needed here to figure out how to validate paths.

On your screenshot I see another problem – we're removing whitespaces from the user input, I no longer think we should do that.

Copy link
Member

Choose a reason for hiding this comment

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

If we are going to support less common values like directory names with trailing spaces, perhaps we should display some kind of outline around the text to clearly mark its bounds, at least in cases where the bounds cannot be visually ascertained.

Copy link
Collaborator

Choose a reason for hiding this comment

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

although a space is a valid path

I learned something new today 😀

adamziel and others added 2 commits July 15, 2024 16:34
Co-authored-by: Bero <berislav.grgicak@gmail.com>
…ingControl.tsx

Co-authored-by: Bero <berislav.grgicak@gmail.com>
@adamziel
Copy link
Collaborator Author

I'll remove this from the board for now. We'll need it sooner than later but let's not occupy a space in a queue.

@adamziel
Copy link
Collaborator Author

UX idea: display select boxes for path selection, not inputs. Options:

  • Plugin
  • Theme
  • Mu-plugin
  • Uploads
  • Wp-content
  • WordPress root
  • Custom path -> this one shows an input

In v2 we could also add a magic "guess" button to infer the role of every path in the tree.

@brandonpayton
Copy link
Member

UX idea: display select boxes for path selection, not inputs. Options:

Nice. This would reduce the opportunity for typos, and selection sounds less stressful to me than having to remember and type the correct path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment