Skip to content

Conversation

@domske
Copy link
Contributor

@domske domske commented Nov 24, 2020

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features) (no changes needed)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features) (no changes needed)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: resolves #20098

What is the new behavior?

The buffering animation is now cropped by a wrapper. It's now not under the the semi-transparent buffer progress bar anymore. No need for solid color. This was the problem. Please see the related issue. ... Never use fixed solid colors. Always use theme colors / variables.

Does this introduce a breaking change?

  • Yes
  • No

Other informations:

I also added a dark mode for the test page to validate the correct use of theme variables. But feel free to remove it (cf131f3). But consider to implement a dark theme for the while tests and docs page in the future.

Before

current

See the solid white color. If we use the --buffer-background with alpha value we have the problem of the buffer circles would be seen through. See:

demo-dark

demo-light

After

This pull-request will fix it:
fixed

@github-actions github-actions bot added the package: core @ionic/core package label Nov 24, 2020
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I did some initial testing and it looks really great. Brandy and I will do more testing and provide feedback.

<div class="progress-buffer-bar" style={{ transform: `scaleX(${finalBuffer})` }}></div>,
<div
class="buffer-circles-wrapper"
style={{ left: `${finalBuffer * 100 * dirScale}%` }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why we are using left rather than a translateX?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's because transform is already used for the animation of the elemen .buffer-circles. If you want to use transform you have to wrap it again in another element. So that you can move the wrapper and have still the transform animation to the buffer dotted element. Or vice versa. (use left for the animation)

@keyframes buffering {
to {
transform: translateX(-10px);
}
}

Some background of this solution. It works because the dotted buffer animation (.buffer-circles) is shifting behind the filled buffer element (.progress-buffer-bar). So that it's not under the semi-transparent .progress-buffer-bar element. (The initial issue.)
I wrapped the .buffer-circles element to use overflow hidden (which requires the position attribute set.) Then moving the wrapper (.buffer-circles-wrapper) after the buffer-bar.
But this movement would affect the animation. The dots animation moves from 0 to -10px using transform. And if you increase the buffer progress, the dots (aka circles) appears to be standing still or moving in another direction and jiggling.
For this reason the dots are moving back inside the wrapper. Synced by the movement of the buffer progress bar and transition. ... This is maybe not the best solution I ever made. But works. I tested it on Windows 10 latest Chrome and iPhone SE 2 iOS 14.2 Safari.

@liamdebeasi liamdebeasi changed the title fix(progress-bar): #20098 Progress bar use theme colors. fix(progress-bar): use correct theme colors Dec 3, 2020
@brandyscarney
Copy link
Member

This appears to break the buffer in rtl mode for some reason, need to investigate

Screen Shot 2021-02-17 at 6 54 48 PM

@domske
Copy link
Contributor Author

domske commented Feb 18, 2021

Thanks for testing. Sorry for the auto-format style changes you have reverted.
However, feel free to close this pull-request. I don't have time at the moment and don't want to continue on this. I'm really sorry. Thank you for your review. ... This is more a workaround. It makes the component a bit more complex and I don't know if there are performance and compatibility issues. Maybe there are other / better solutions ... Personally, I would completely rewrite the component. And craft a new one by inspiring from other frameworks and respecting the current Ionic component. As I posted in the ref issue, the Google team (Angular Material) also failed on the styles. Anyway...

For the future, I would like to suggest that Ionic integrate a dark mode and make all components fully compatible with both themes. Including the tests. :)

@brandyscarney
Copy link
Member

Thanks @domske! We can get this finished up, we appreciate your work on it!

The progress bar was originally added by a contributor and dark mode was an oversight. We do actually have tests for different themes for the components at http://localhost:3333/src/themes/test/css-variables and have made a ton of improvements on many components once adding support for dark mode.

Right now we need to make improvements to our tests and our screenshot tool as it is taking hundreds of images and this can take upwards of 20 minutes to run so we are looking into ways to speed this process up in order to add more images.

@github-actions github-actions bot added package: angular @ionic/angular package package: vue @ionic/vue package labels Feb 22, 2021
@liamdebeasi
Copy link
Contributor

Hey there,

I am going to close this PR in favor of #22957. This branch was out of date with master and it was easier to just remake the PR.

I will definitely still give you co-author credit when merging.

Thank you again!

@liamdebeasi
Copy link
Contributor

This change has been merged via #22957. Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Progress bar doesn't use theme colors

3 participants