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

Simplify Modal Config #33249

Merged
merged 2 commits into from
Apr 6, 2021
Merged

Simplify Modal Config #33249

merged 2 commits into from
Apr 6, 2021

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Mar 3, 2021

No description provided.

@GeoSot GeoSot requested a review from a team as a code owner March 3, 2021 00:18
@GeoSot GeoSot changed the title Simplify Modal Config WIP: Simplify Modal Config Mar 3, 2021
@XhmikosR XhmikosR marked this pull request as draft March 3, 2021 05:40
@GeoSot GeoSot changed the title WIP: Simplify Modal Config Simplify Modal Config Mar 3, 2021
@GeoSot GeoSot marked this pull request as ready for review March 3, 2021 22:04
Copy link
Collaborator

@rohit2sharma95 rohit2sharma95 left a comment

Choose a reason for hiding this comment

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

Thank you @GeoSot ❤️

js/src/modal.js Outdated

data = new Modal(target, config)
}
const data = Data.get(target, DATA_KEY) || new Modal(target, Manipulator.getDataAttributes(this))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is not mentioned anywhere in the docs that the data API reads the configuration from the data attributes of the toggle button also 🤯

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not either tested. I am in favor to omit it. If you agree so. To my eyes it's not a valid behavior

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be omitted IMO if it is not a breaking change 🤔

js/src/modal.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants