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(progress-bar): use correct theme colors #22957

Merged
merged 3 commits into from
Feb 23, 2021
Merged

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Feb 22, 2021

TODO: Give co-author credit to domske

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • 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?

  • Don't place a white background on the buffer bar to allow theming to work properly

Does this introduce a breaking change?

  • Yes
  • No

Other information

Before After
before after

@github-actions github-actions bot added the package: core @ionic/core package label Feb 22, 2021
@brandyscarney
Copy link
Member

The dots appear pretty visibly on the regular buffer bar (but not reversed):

@liamdebeasi liamdebeasi merged commit 26285bb into master Feb 23, 2021
@liamdebeasi liamdebeasi deleted the progress-theme branch February 23, 2021 18:13
@domske
Copy link
Contributor

domske commented Feb 23, 2021

Hmm, something went wrong. In my original commit, the dots are moved synchron with the progress bar. And the dots container was overflow hidden. So that the dots are smoothly appear and disappear on the edges. But with the current state it looks like this (The dots shines through. And are not animated sync.)

demo

And the dots suddenly disappear:
demo

Brandy, your GIF: is that an optical illusion or are the dots going in the wrong direction?

I'll have a quick look ...

TL;DR: The trick is that the dots are in a container with overflow hidden. And just animated. The dot wrapper position is behind the progress / buffered. And moved synchronously with the progress bar to prevent gaps or overlap.

@liamdebeasi
Copy link
Contributor Author

liamdebeasi commented Feb 23, 2021

Thanks, I forgot a transition. I will resolve that here: #22964

Because we are animating transform and width, they won't be perfectly in sync, but the overlap/gap should not be nearly as noticeable after my PR.

For the dots disappearing, that has always been there. The issue is just more pronounced because the dots no longer go behind the buffer. One idea is to animate background position rather than transform, but there may be some performance issues associated with that.

edit: Also the dots disappearing also seems to be in the Vuetify version as well.

@domske
Copy link
Contributor

domske commented Feb 23, 2021

Ok, but if the wrapper is overflow hidden, the dots will also not behind the buffer. (as in my original commit).
demo
(This was my initial fix. Ok, one minor issue is the abruptly appearing points. But disappear smoothly. And the animation is sync. Could be fixed by increase the width. It's safe because overflow hidden.)

Maybe the wrapper has been removed?
But I not yet viewed my orginal and the current state of the source code. So please don't get confused by me. ^^

@domske
Copy link
Contributor

domske commented Feb 23, 2021

btw. the live reload does not update changes in scss. I have to restart on every change by npm start. Changes in jsx works. Is there a trick? I have the same issue on my projects with stencil. :/ Is this a known issue with stencil? I'm on WIn 10 with Chrome. Nothing special. Do you have the same problems? (http://localhost:3333/src/components/progress-bar/test/basic)
The compiler is triggered on save, but the browser does not update. Maybe scss has to rebuild completely? It's not fun to work with.

@brandyscarney
Copy link
Member

@domske Yes we run into similar issues with the stencil build, I believe it is this issue ionic-team/stencil#2635

As a workaround if you make a change in progress-bar.scss you can update progress-bar.ios.scss to trigger the style changes.

@domske
Copy link
Contributor

domske commented Feb 23, 2021

@brandyscarney Thanks, It works. But updating the .(ios|md).scss breaks the styles of the site and I have to press the refresh button of the browser. sigh anyway ...

@domske
Copy link
Contributor

domske commented Feb 23, 2021

Back to the issue: Don't set width / height to e.g. 100% when using left/right/top/bottom. It's a style conflict / overwrite.
Example:

left: -10px;
right: -10px;
width: 100%;

will not work. The element is now left: -10px and width 100% instead of 100% + 20px.
And left/right/top/bottom 0 (zero) and width/height 100% works. But only because of same size. But avoid using both.

@liamdebeasi
Copy link
Contributor Author

I wonder if it makes sense to keep the container that was in the old PR, set overflow: hidden and just change the width of that container. It might make for a nicer effect.

The reason why we changed the PR was because it was animating left which has some performance implications. I'll see if there's a better way to do this later this week.

@domske
Copy link
Contributor

domske commented Feb 23, 2021

The only idea with the overflow-hidden container, is that the dots can move behind the progress bar without shining through.
So, without this container - and only change the width, results in a aprupt disapearing dot background. Because you cannot move some pixels forward without overlay the progress bar. ...
I test some other solution (simpler) at this moment. But it's a bit tricky this little component. ^^

Where do you benchmark the style performance? I know that transform and other 3d accelerated stuff is better. Aber is there a way to measure things like left vs translateX or scaleX vs width, etc? Only in DevTools?
For example, what will happen if we do some flex stuff for the dots element. Or all three elements relative. So that the width is depending from each other. - Also good to know is that e.g. translateX 100% is not left 100%. Transform uses the own element size, and the left uses the parent size. ... So it's not always easy to pick translateX over left.
see demo

@liamdebeasi
Copy link
Contributor Author

Yeah, I like that overflow hidden container idea.

Re: benchmarking style performance-- this is a shameless plug but I go over the tools I use here: https://www.youtube.com/watch?v=A5a15NrcGoM

Using left probably isn't the end of the world performance-wise but we prefer to stick with transform where possible.

@domske
Copy link
Contributor

domske commented Feb 24, 2021

So fixed #22965 😓 But feel free to modify.
I reverted to tag 5.5.4 (latest release at this moment) and made the (same/similar) fix again.
WIth transform animation. :) Another, more simpler (without double container) solution requires other layout and animated attributes (not transform only). I also wonder if we could make the background fixed, so that we don't need a second container (move back to keep animation flow).
The transition is on fast progress movements not 100% perfect. But about 99%.
And note that finalBuffer !== 1 shows/hides the element directly. So also a minor transition issue.
This is moved to display: none btw. Because otherwise the elements jumps due to transition.
But I think this can be release. What do you think?
You can do whatever you want. Seriously, I wouldn't feel offended. :)

If you wonder: Why two containers? Well, to use translate and not left. Note that you have to move the container forward and the child backwards to balance the movement. otherwise the animation glitches.

I also just flip the component with scaleX(-1). I think it’s easier than handling each child separately. So you have not care about it. No more weird bugs in reserved state. The basic tests looks good. Tested in Chrome.

@liamdebeasi
Copy link
Contributor Author

Thank you! I will take a look at the PR.

cerkiner added a commit to cerkiner/ionic that referenced this pull request Mar 2, 2021
* master: (284 commits)
  fix(label): only show placeholder with floating label when focused (ionic-team#22958)
  feat(react): add react hooks to control overlay components (ionic-team#22484)
  feat(vue): add composition API ionic lifecycle hooks (ionic-team#22970)
  chore(): run build
  5.5.5
  fix(vue): account for event name changes in vue 3.0.6+
  fix(react, vue): navigating using ion-back-button now selects correct page (ionic-team#22974)
  fix(progress-bar): use correct theme colors in dark mode (ionic-team#22965)
  fix(progress-bar): add width transition (ionic-team#22964)
  fix(vue): prevent race conditions when opening overlays (ionic-team#22883)
  fix(progress-bar): use correct theme colors in dark mode (ionic-team#22957)
  feat(searchbar): add showClearIcon property (ionic-team#22759)
  test(): update theming test with latest dark mode values (ionic-team#22956)
  chore(): update package-lock to account for npm 7.5.3 bug fix (ionic-team#22963)
  fix(header): collapsed toolbar is no longer incorrectly shown when using ion-refresher (ionic-team#22937)
  fix(react): onIonTabsWillChange and onIonTabsDidChange event handlers are now properly bound to IonTabs (ionic-team#22233)
  fix(refresher): add correct dark mode styles (ionic-team#22639)
  feat(custom-elements): add experimental custom elements build  (ionic-team#22863)
  fix(a11y): improve support for ids with special characters when getting label element (ionic-team#22680)
  chore(): update code of conduct (ionic-team#22619)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Progress bar doesn't use theme colors
3 participants