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.

Sign up for GitHub

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

Jump to bottom

Illustration picker #18565

Merged
merged 3 commits into from
Dec 20, 2024
Merged

Illustration picker #18565

merged 3 commits into from
Dec 20, 2024

Conversation

AdrienClairembault
Copy link
Contributor

@AdrienClairembault AdrienClairembault commented Dec 17, 2024 •
edited
Loading

Checklist before requesting a review

  • I have performed a self-review of my code.
  • I have added tests that prove my fix is effective or that my feature works.

Description

Add a new illustration picker field.

Screenshots and videos

Selecting an icon: (open the full sized video in another tab to see more clearly)

demo

Illustration input:

image

Modal:

image

Searching for an icon (slowed down):

search

Using the pagination (slowed down):

pagination

Search with no results:

image

@AdrienClairembault AdrienClairembault self-assigned this Dec 17, 2024
AdrienClairembault
AdrienClairembault commented Dec 17, 2024
View reviewed changes
css/includes/components/_richtext.scss
.tox.tox-tinymce {
border: var(--tblr-border-width) solid var(--tblr-border-color);
.tox.tox-tinymce:not(.content-editable-tinymce .tox.tox-tinymce) {
border: var(--tblr-border-width) solid var(--tblr-border-color) !important;
Copy link
Contributor Author

@AdrienClairembault AdrienClairembault Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Somewhat unrelated, I've noticed that some of tiynmce styles were not applied correctly (since I was trying to display a form field next to a tinymce field, the differences were very noticeable).

Fixed it and made the selector more precise as we don't want to impact the special .content-editable-tinymce class.

AdrienClairembault
AdrienClairembault commented Dec 17, 2024
View reviewed changes
.stylelintrc.js
@@ -17,7 +17,6 @@ module.exports = {
"color-function-notation": null, // DISABLE: Expected modern color-function notation
"declaration-block-no-redundant-longhand-properties": null, // DISABLE Expected shorthand property "flex-flow"
"media-feature-range-notation": "prefix",
"selector-not-notation": "simple", //DISABLE Expected complex :not() pseudo-class notation
Copy link
Contributor Author

@AdrienClairembault AdrienClairembault Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I need a complex selector for a case that do not seems possible with simple selectors.

I've removed this rule, but it requires us to remove usages of simple selectors so I've had to fix a few existing css rules using the automated --fix command.

See https://stylelint.io/user-guide/rules/selector-not-notation/.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't seem to be an issue. All officially supported browsers support selector lists.
https://caniuse.com/css-not-sel-list

@AdrienClairembault AdrienClairembault marked this pull request as ready for review December 17, 2024 16:29
@AdrienClairembault @cconard96
Apply label suggestion
a1e83db 
Co-authored-by: Curtis Conard <cconard96@gmail.com>
cconard96
cconard96 reviewed Dec 18, 2024
View reviewed changes
Copy link
Contributor

@cconard96 cconard96 left a comment

Choose a reason for hiding this comment

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

Stuck to mainly a functional review.

  • The illustrations in the picker slightly overflow the bottom of their container on larger screens.
  • On smaller screens, the same layout of illustrations in the picker is used but the actual illustrations get smaller. Makes sense to keep the same illustration size and simply show less per row.

@AdrienClairembault
Copy link
Contributor Author

AdrienClairembault commented Dec 19, 2024 •
edited
Loading

On smaller screens, the same layout of illustrations in the picker is used but the actual illustrations get smaller. Makes sense to keep the same illustration size and simply show less per row

Good catch, forgot to update the breakpoints. Should be fixed now.

Render on phones:

image

Render on tablets:

image

@AdrienClairembault
Copy link
Contributor Author

AdrienClairembault commented Dec 19, 2024 •
edited
Loading

The illustrations in the picker slightly overflow the bottom of their container on larger screens.

Can you please share a screenshot of this issue ? I don't seems to reproduce it on my side.

@cconard96
Copy link
Contributor

The illustrations in the picker slightly overflow the bottom of their container on larger screens.

Can you please share a screenshot of this issue ? I don't seems to reproduce it on my side.

I cannot recreate the issue either now. I'm thinking it could have just been a random issue in Chrome. It wouldn't be the first time.

orthagh
orthagh approved these changes Dec 20, 2024
View reviewed changes
Copy link
Contributor

@orthagh orthagh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

On the feature itself and the rendering -> nothing to say, it's very good.
While testing, I think we need a default neutral icon.
"Request a service" can't be this default.
I don't have an idea yet about how we should represent the concept

@trasher trasher merged commit 6344e60 into glpi-project:main Dec 20, 2024
9 checks passed
@trasher
Copy link
Contributor

trasher commented Dec 20, 2024

On the feature itself and the rendering -> nothing to say, it's very good. While testing, I think we need a default neutral icon. "Request a service" can't be this default. I don't have an idea yet about how we should represent the concept

Maybe open a ticket or add a task in project's backlog so it won't be firget

@AdrienClairembault
Copy link
Contributor Author

On the feature itself and the rendering -> nothing to say, it's very good. While testing, I think we need a default neutral icon. "Request a service" can't be this default. I don't have an idea yet about how we should represent the concept

Maybe open a ticket or add a task in project's backlog so it won't be firget

Done.

@AdrienClairembault AdrienClairembault mentioned this pull request Dec 23, 2024
Merged
1 task
@AdrienClairembault AdrienClairembault deleted the illustration-picker branch January 13, 2025 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cconard96 cconard96 cconard96 approved these changes

@orthagh orthagh orthagh approved these changes

@trasher trasher trasher approved these changes

@cedric-anne cedric-anne Awaiting requested review from cedric-anne

Assignees

@AdrienClairembault AdrienClairembault

Labels
Forms
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AdrienClairembault @cconard96 @trasher @orthagh