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

fix(Form/Input/Date): Make the glyphicon clickable #699

Merged
merged 1 commit into from
Oct 16, 2020

Conversation

fdescamps
Copy link
Contributor

Related issue

Description of the issue

When you use the Form/Input/Date component, you are not able to click on the glyphicon in the input field. You have to click somewhere else in the input field.

Person(s) for reviewing proposed changes

@gcruchon @guillaumechervet @guillaume-chervet @guillaumechervetaxa @samuel-gomez @samuel-gomez-axa @Cleclemi28 @youf-olivier @Gparquet

Important

Before creating a pull request run unit tests

$ npm test

# watch for changes
$ npm test -- --watch

# For a specific file (e.g., in packages/context/__tests__/command.test.js)
$ npm test -- --watch packages/action

gcruchon
gcruchon previously approved these changes Oct 7, 2020
Copy link
Contributor

@gcruchon gcruchon left a comment

Choose a reason for hiding this comment

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

Thanks foe this PR! I pulled it and it does the job.

However, modals have z-index > 1000. If it's possible to add a date input field in a modal, I guess it would not work. Not sure if such situation can occur, though... @samuel-gomez / @guillaumechervet : what do you think?

@fdescamps
Copy link
Contributor Author

Hi @samuel-gomez / @guillaumechervet,

I permit myself to ask you if you saw @gcruchon question ?

Thanks by advance for reviewing my first PR on this toolkit ;)

@arnaudforaison
Copy link
Contributor

For me z-index it's not a good practice. I think you may refactor it to remove input z-index

@arnaudforaison arnaudforaison added the Type: Bug Design graphics bugs label Oct 11, 2020
@gcruchon
Copy link
Contributor

I can't agree more with you, @arnaudforaison, about z-index. They are, in deed, bad practices.

However, there are 33 of them in the packages. I think there should be a dedicated issue to remove them all at once. Moreover, I tend to think it may lead to some breaking changes (some HTML element may have to be moved around).

This issue should also ask to improve the document to clearly state that z-index are not welcome, what do you think?

@arnaudforaison
Copy link
Contributor

Yes I agree that's a general review of z-index must be necessary.

But why to no intent here as 1st sample for other ?

@gcruchon
Copy link
Contributor

My experience says that, z-index are like !important, if you try to remove one, you'll have to remove them all. I doubt the "sample" as a solution that will work with all the combinations of component. But maybe @fdescamps wants to try?

@@ -84,7 +84,7 @@ $border-radius-width: 3px;
}

&__input-container {
z-index: 2;
z-index: 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of changing this z-index, just leave it "as-is" and remove z-index on .glyphicon-calendar (line 17). Should do the trick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I test it asap, thanks for the review @gcruchon and @arnaudforaison. It is quite interesting !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

indeed, this solution may not work all the time, you have to test in many cases. but I don't think using z-indexes is bad practice.

@fdescamps
Copy link
Contributor Author

Hi,

I test "Instead of changing this z-index, just leave it "as-is" and remove z-index on .glyphicon-calendar (line 17). Should do the trick." And it works !

Thanks @gcruchon

Copy link
Contributor

@guillaume-chervet guillaume-chervet left a comment

Choose a reason for hiding this comment

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

So lets try it on ours application ^^

@arnaudforaison arnaudforaison merged commit 1eb92cc into AxaFrance:master Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Design graphics bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants