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 panelWidth prop to all filters + Fix contentHeight being limited … #201

Closed
wants to merge 2 commits into from

Conversation

Bartmr
Copy link
Collaborator

@Bartmr Bartmr commented Jan 17, 2022

This PR adds a prop to set the width of the Filter panel, and fixes an issue where contentHeight would not work if it surpassed '290px', as it was being limited by the filters max-with:290px style
Screenshot from 2022-01-17 13-01-16
Screenshot from 2022-01-17 13-00-58

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request does not contain a valid label. Please add one of the following labels: major, minor, patch, skip-release

@Bartmr Bartmr added the minor Increment the minor version when merged label Jan 17, 2022
@Bartmr Bartmr requested a review from nicmosc January 17, 2022 13:06
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains a valid label.

Copy link
Member

@nicmosc nicmosc left a comment

Choose a reason for hiding this comment

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

@Bartmr why remove the max-height? In what context do you need more than the predefined height set by the panel? Content will scroll anyway
Also, why the need for panelWidth? The content should adapt to the children width, and if you really need more spacing, either check if it's something we should have as a default (set by the component) or use the data-element property to target it and set the width like you're doing here, but do it from Mosaic, as it's a one off use case i believe.

I also wonder if we've ever used the contentHeight prop, which is why i dont see the point of this PR, can you explain?

@Bartmr
Copy link
Collaborator Author

Bartmr commented Jan 17, 2022

@nicmosc The default height of the filter only lists 4 items when the design specifies at least 10 before showing the scroll (which surpasses the default / max of 290px)
image

Also, the current filter does not expand depending on the children, but instead wraps them.

I can override by attribute, but this issue might come up in other filters, like the items length not being enough to also expand the search bar and fully show the placeholder: I think it would be a cleaner solution to offer that option more clearly as a prop.

@nicmosc
Copy link
Member

nicmosc commented Jan 17, 2022

@Bartmr this seems to me like a combination of things which makes the filter panel look small (search + clear). If the designs were changed i think we can probably simply increase the max height within the component so that more is shown, nothing wrong with that.

Regarding the width of the panel, i think it really shouldn't be up to the implementer of the feature to have to take care of how a component should display itself. The goal of drylus is to reduce the amount of uncertainty a developer has when implementing the components, and here the ideal thing is that the component displays itself the best way depending on what is given to it. If some override is necessary, we can go out of our way to make this happen. Remember that to Drylus, the developers are the clients, meaning the cleanest solution is always the one that requires the least effort from someone using the library. So in this case for me, the cleanest solution is to have the panel width expand to the size of the children, once again though, let's have a max width set so that things don't get out of control. We can use some CSS to make the text wrap if it's over a certain width (this should be possible with simple CSS without JS, but you might need to investigate).

@Bartmr
Copy link
Collaborator Author

Bartmr commented Jan 17, 2022

I was able to get the styles I needed with this override. But it's still a bit unclear how to override a style, specially if the order of elements inside the component implementation change

const styles = {
  filter: css`
    > div {
      max-height: initial;
    }

    [data-element='panel'] {
      max-height: 460px;
      min-width: max-content;
    }
  `,
};

I will close the PR

@Bartmr Bartmr closed this Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants