-
Notifications
You must be signed in to change notification settings - Fork 13.4k
feat: add ionic theme architecture #29132
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
Conversation
While further evaluating this approach, Ionic already has a concept of "platforms" that refer to iOS, Android, iPhone, iPad, etc. This language is confusing with trying to use platform for just the specific iOS and Android behaviors we have baked into components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following props mention being mode-dependent in their docs comments, but as far as I can tell are now theme dependent and thus need the comments updated:
translucenton action-sheet, alert, card-header, fab-button, footer, header, loading, popover, tab-bar, toastcollapseon footer, headerfillon input, textareadetailon itempresentingElementon modal- The
arrowshadow part and prop, plusalignment, on popover cancelButtonIcon,cancelButtonText, andsearchIconon searchbarfill,toggleIcon, andexpandedIconon selectprimaryandsecondaryshadow parts on toolbar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great list 👍
presentingElementis still based on mode detection currently. We may adjust when working on overlays as part of the new theme.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On closer inspection, presentingElement is dependent on both mode and theme currently. It is used to tell whether a card modal should be rendered here, which is then used to configure the classes modal-default and modal-card. The styling for these classes is theme-dependent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this hasn't been done already, could be worth adding a note to the ticket for writing docs about theme vs. mode to ensure the config option descriptions are updated to mention the right prop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to take a closer look at the implementation in ionic-global.ts and test it out but I reviewed all of the other files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about the multiple rounds of reviews, didn't know Github would submit some of my comments early 😅
I also have a couple earlier comments that haven't been fully addressed yet, re: presentingElement on modal and collapse on buttons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment is a suggestion. Regardless of what you decide, request me when it's ready.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we create another ticket to look into the presentingElement issue I mentioned in #29132 (comment), or update the existing one for collapse to cover it as well? Otherwise this LGTM.
Co-authored-by: Maria Hutt <thetaPC@users.noreply.github.com>
@amandaejohnston created FW-6049 to track. Feel free to create spikes for tasks like that as you think they make sense, especially if you have context from your comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, woot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As well as the inline comments, it looks like the following components are missing an ionic theme in the styleUrls:
- Datetime Button
- Picker Column Option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Co-authored-by: Brandy Carney <brandyscarney@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Issue number: Internal
What is the current behavior?
What is the new behavior?
Adds the base architecture to add a new theme configuration to Ionic Framework components.
ionictheme.themeandmodeindependently to control look and feel of a component.Test infrastructure has been updated to add support for testing the theme configuration with Playwright.
themestest configuration has been renamed topalettesThis PR is just the initial effort to decouple Ionic's architecture to separate look and feel and allow our dev team to start introducing the new component appearance to the UI. There will be additional changes required to completely add support for the Ionic theme. These changes are targeted against the
nextbranch and are not expected to be used in a production environment at this time.Does this introduce a breaking change?
Other information