pnp / sp-dev-fx-controls-react

Reusable React controls for SPFx solutions
https://pnp.github.io/sp-dev-fx-controls-react/
MIT License
382 stars 379 forks source link

LocationPicker width #1125

Closed PathToSharePoint closed 2 years ago

PathToSharePoint commented 2 years ago

Category

[ ] Enhancement

[ ] Bug

[x] Question

Version

Please specify what version of the library you are using: [3.5.0]

Expected / Desired Behavior / Question

How can I force the width of the LocationPicker? Or have it fit in its container? I thought the components were built on Office UI Fabric, but I don't see a style prop.

Observed Behavior

Here is what it looks like in my test, I am unable to change the address once it is set: image

Thanks!

ghost commented 2 years ago

Thank you for reporting this issue. We will be triaging your incoming issue as soon as possible.

joelfmrodrigues commented 2 years ago

Hi @PathToSharePoint , I just tested this and there is no option to style. Would you be interested in submitting a PR? It should be a simple change and similar to the properties available in the People picker control

PathToSharePoint commented 2 years ago

@joelfmrodrigues If you say it's simple, I'll give it a shot 😊

joelfmrodrigues commented 2 years ago

@PathToSharePoint that's great ☺️ I have assigned this issue to you (no pressure), but let me know if you change your mind and I can remove it. It's only to try to prevent more than one person working on it

PathToSharePoint commented 2 years ago

I took a closer look, and here is the issue I am trying to address (no pun intended): if the address is too long, it pushes the delete control (cross) outside of the viewport, and it becomes impossible to edit the field.

I think the easiest way to fix the issue is to crop the address and append an ellipsis, like it's done in the list picker. Another way would be to display the address on multiple lines, like in the screenshot below.

It seems that the control is built on the Fluent UI Persona component, let me see what styling it allows.

image

PathToSharePoint commented 2 years ago

Looking at the code, I found in LocationPicker.module.scss a class name persona. That should allow me to inject some additional styling, without major refactoring.

PathToSharePoint commented 2 years ago

@ravichandran-blog and @AJIXuMuK, I see that you are the authors of this control.

I plan to remove width: fit-content; in LocationPicker.module.scss as it seems to be what's causing the issue. Do you see any objection?

.pickerItemContainer {
  display: flex;
  border-style: solid;
  border-color: "[theme: themePrimary, default: #0078d7]";
  border-radius: 22px;
  min-width: 16px;
  border-width: 2px;
  width: fit-content;
}

Once I remove it, the text gets truncated and the control fits in the width. For context, all my screenshots are from the SPFx Property Pane, which has a width of around 300 px.

When the control loses focus, a different component is rendered and everything looks fine:

ravichandran-blog commented 2 years ago

@PathToSharePoint Awesome! go ahead

PathToSharePoint commented 2 years ago

@ravichandran-blog PR submitted!

Unrelated to this issue, the only event handler I see is onChange, I didn't see an event handler for clear. If you clear the control, the app will still retain the previous value until you pick a new location. Am I missing something?

PathToSharePoint commented 2 years ago

And I just ran into another issue, the location finder gives me a 401 when I run it from the root site tenant.sharepoint.com/_layouts/15/workbench.aspx. I'll need to do more testing before opening an issue but wanted to pass the info. [edit] The issue might be related to Web.ServerRelativeUrl.

PathToSharePoint commented 2 years ago

I have posted my sample to Github, here is the link in case you want to test: https://github.com/PathToSharePoint/React-PPP-PnP-Controls

joelfmrodrigues commented 2 years ago

@PathToSharePoint awesome updates :) PS: I was able to recreate the issue on the root site as well.

ravichandran-blog commented 2 years ago

Root site issue fixed https://github.com/pnp/sp-dev-fx-controls-react/pull/1162

joelfmrodrigues commented 2 years ago

@ravichandran-blog many thanks @ravichandran-blog , merged

PathToSharePoint commented 2 years ago

So the final touch would be to accommodate an onClear event handler similar toonChange.

Does this look right @ravichandran-blog

  private onIconButtonClick = () => {
    this.setState({ currentMode: Mode.empty, selectedItem: null });
    if (this.props.onClear) {
      this.props.onClear();
    }
  }

I prefer to ask as I am more familiar with function components, not so much with this.setState.

PathToSharePoint commented 2 years ago

On second thoughts, we could reuse the onChange event handler:

  private onIconButtonClick = () => {
    this.setState({ currentMode: Mode.empty, selectedItem: null });
    if (this.props.onChange) {
      this.props.onChange(null);
    }
  }
PathToSharePoint commented 2 years ago

I have submitted a PR with the second option: #1165.