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

Merged
merged 7 commits into from
Sep 14, 2024

Conversation

adamziel
Copy link
Collaborator

@adamziel adamziel commented Jul 12, 2024

Motivation for the change, related issues

Adds a PathMappingControl React component:

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

The 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.

@adamziel
Copy link
Collaborator Author

adamziel commented Sep 14, 2024

There are improvements we could do here, but I'll go ahead and merge as it's not used anywhere and I'd like to play with it in a draft branch.

@adamziel adamziel merged commit 8778025 into trunk Sep 14, 2024
5 checks passed
@adamziel adamziel deleted the add-path-mapping-control branch September 14, 2024 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants