Replaced BottomBar with BottomToolBar #4491
Conversation
|
Hi @Wck-iipi, thank you for the contribution. I'm sorry for confusion but it's the other bottom bar component that needs to be removed. You can see more here #4351 (comment) and I've just updated the issue description to reflect that #2008 |
|
@MisRob I have made the desired changes. I'm sorry I should've looked at previous PR more carefully. I'll be more careful in future. |
|
Oh no, @Wck-iipi, that's on me, this should definitely be mentioned in the issue. We really do not expect anyone to study previous PRs, unless we recommend it for a specific reason. Thank you for your patience and flexibility. |
There was a problem hiding this comment.
Hi @Wck-iipi it looks like the linting check is failing. Could you run pre-commit as described in the Kolibri dev docs?
|
@LianaHarris360 I have fixed the linting issue |
contentcuration/contentcuration/frontend/channelList/views/Channel/CatalogList.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/shared/views/FullscreenModal.vue
Outdated
Show resolved
Hide resolved
| v-if="selecting" | ||
| clipped-left | ||
| color="white" | ||
| flat | ||
| data-test="toolbar" | ||
| :height="$vuetify.breakpoint.xsOnly ? '72px' : '56px'" |
There was a problem hiding this comment.
This height prop isnt working now, and this is happening on mobile screens, the bottom bar now doesnt have an appropiate height:
Before:
After:
One thing we can do is that the BottomBar component receives an appearanceOverrides, where we can pass overrides styles to the bar, and we can end with something like:
<BottomBar :appearanceOverrides=" { height: ... } ">
There was a problem hiding this comment.
I have made the desired changes and tested it on the screen above.
| width: 100%; | ||
| height: 64px; | ||
| height: v-bind('appearanceOverrides.height'); |
There was a problem hiding this comment.
Lets have this a little more generic, instead of setting the specific property, we can have this css rule with 64px, but we can pass the appearanceOverrides object to the wrapper div like this:
<div class="bottom-bar pa-2" :style="appearanceOverrides">
<slot></slot>
</div>This way if the implementers wants to override any other property apart fromm the height can do it without having to update the component.
| // eslint-disable-next-line kolibri/vue-no-unused-properties | ||
| appearanceOverrides: { | ||
| type: Object, | ||
| default: () => ({ height: '64px' }), |
There was a problem hiding this comment.
With the proposed change on the style bellow, this default value will not be required. And this eslint-disable line can be erased too 👐
There was a problem hiding this comment.
@AlexVelezLl I have generalized appearanceOverrides
Summary
Description of the change(s) you made
Replaced BottomBar with BottomToolBar and removed BottomBar
Manual verification steps performed
I tested the EditModal.vue page, it runs as expected. Also, I ran tests and all passed.
Screenshots (if applicable)
Does this introduce any tech-debt items?
Reviewer guidance
How can a reviewer test these changes?
Please check if StagingTreePage works, I tried finding BottomBar here but couldn't.
Are there any risky areas that deserve extra testing?
References
Fixes #2008
Comments
Contributor's Checklist
PR process:
CHANGELOGlabel been added to this PR. Note: items with this label will be added to the CHANGELOG at a later timedocslabel has been added if this introduces a change that needs to be updated in the user docs?requirements.txtfiles also included in this PRStudio-specifc:
notranslateclass been added to elements that shouldn't be translated by Google Chrome's automatic translation feature (e.g. icons, user-generated text)pages,components, andlayoutsdirectories as described in the docsTesting:
Reviewer's Checklist
This section is for reviewers to fill out.
yarnandpip)