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

Extract Component config functionality to a separate class #33872

Merged
merged 2 commits into from
Dec 10, 2021

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented May 7, 2021

As we use the config functionality in many places, we maybe can use it extending a base Config Class

js/src/util/config-class.js Outdated Show resolved Hide resolved
@GeoSot GeoSot force-pushed the gs-export-config-functionality branch from 8007f0f to 5a45eff Compare May 7, 2021 23:05
@GeoSot GeoSot force-pushed the gs-export-config-functionality branch 2 times, most recently from 4b05ec8 to 7afb15f Compare May 10, 2021 00:04
@GeoSot
Copy link
Member Author

GeoSot commented May 19, 2021

@twbs/js-review
I need some help here.

After some discussion I have concluded to the following.

Config can be three different things:

  • An abstract with mutable getters (as it is now on Tooltip|Popover, and can be changed by developers)
  • An abstract with immutable getters (private methods)
  • A separate Class|object that any component can initialize

The third option seems to be more strict (and with some difficulties on its implementation)
Both first and second are very close too our existing codebase.
IMO the first is ok as we give the opportunity to devs that use Bootstrap to extend/change some Defaults on the fly, with the second to be a more professional choice than first, as it can give the same freedom, only after extending a class that already extends the Config abstract (better for third party plugins and custom impersonations)

Any opinions are welcome

@rohit2sharma95
Copy link
Collaborator

So choosing the first option seems good, but the APIs should be kept private initially IMO. Since that's the fully internal thing.

@GeoSot GeoSot force-pushed the gs-export-config-functionality branch 4 times, most recently from f100a38 to 63ea869 Compare June 7, 2021 13:06
@GeoSot GeoSot force-pushed the gs-export-config-functionality branch from 63ea869 to 2ee0975 Compare July 27, 2021 07:56
@GeoSot GeoSot force-pushed the gs-export-config-functionality branch 3 times, most recently from dc0142b to e583876 Compare August 4, 2021 13:43
@GeoSot GeoSot force-pushed the gs-export-config-functionality branch from e583876 to f08312e Compare September 10, 2021 23:22
@GeoSot GeoSot force-pushed the gs-export-config-functionality branch from 2c79c96 to 07bec70 Compare October 5, 2021 23:13
@GeoSot GeoSot force-pushed the gs-export-config-functionality branch 2 times, most recently from 41e34b9 to e7fe92f Compare October 14, 2021 00:15
@GeoSot GeoSot force-pushed the gs-export-config-functionality branch from e7fe92f to 0edddc9 Compare November 5, 2021 00:07
@GeoSot GeoSot force-pushed the gs-export-config-functionality branch from 0edddc9 to 397a9ee Compare November 23, 2021 00:19
@GeoSot GeoSot force-pushed the gs-export-config-functionality branch 3 times, most recently from fd75b76 to 00fed52 Compare December 5, 2021 18:12
@XhmikosR XhmikosR force-pushed the gs-export-config-functionality branch 2 times, most recently from 6809fd3 to 839e89f Compare December 7, 2021 14:27
@XhmikosR XhmikosR marked this pull request as ready for review December 10, 2021 14:24
@XhmikosR XhmikosR requested a review from a team as a code owner December 10, 2021 14:24
@XhmikosR XhmikosR force-pushed the gs-export-config-functionality branch from 2f47950 to c4e5c44 Compare December 10, 2021 14:24
@XhmikosR XhmikosR force-pushed the gs-export-config-functionality branch from 74114d5 to 8859470 Compare December 10, 2021 14:55
@XhmikosR XhmikosR force-pushed the gs-export-config-functionality branch from 8859470 to fa12080 Compare December 10, 2021 14:56
@XhmikosR XhmikosR changed the title Extract Config functionality Extract Component config functionality to a separate class Dec 10, 2021
@XhmikosR XhmikosR merged commit 886b940 into main Dec 10, 2021
@XhmikosR XhmikosR deleted the gs-export-config-functionality branch December 10, 2021 16:18
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.

5 participants