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.

Sign up for GitHub

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

Update Topics page header navigation #8894

Merged
merged 1 commit into from
Dec 14, 2021
Merged

Update Topics page header navigation #8894

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion kolibri/plugins/learn/assets/src/routes/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,22 @@ export default [
};
},
},
{
// Handle redirect for links without the /folder appended
path: '/topics/t/:id',
redirect: '/topics/t/:id/:subtopic?/folders',
handler: (toRoute, fromRoute) => {
if (unassignedContentGuard()) {
return unassignedContentGuard();
}
// If navigation is triggered by a custom navigation updating the
// context query param, do not run the handler
if (toRoute.params.id === fromRoute.params.id) {
return;
}
showTopicsTopic(store, { id: toRoute.params.id, pageName: toRoute.name });
},
},
// Have to put TOPICS_TOPIC_SEARCH before TOPICS_TOPIC to ensure
// search gets picked up before being interpreted as a subtopic id.
{
Expand All @@ -134,7 +150,7 @@ export default [
},
{
name: PageNames.TOPICS_TOPIC,
path: '/topics/t/:id/:subtopic?',
path: '/topics/t/:id/:subtopic?/folders',
handler: (toRoute, fromRoute) => {
if (unassignedContentGuard()) {
return unassignedContentGuard();
Expand Down
80 changes: 19 additions & 61 deletions kolibri/plugins/learn/assets/src/views/TopicsPage.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,55 +64,30 @@
</KGrid>
<!-- Nested tabs within the header, for toggling sidebar options -->
<!-- larger screens -->
<div class="tabs">
<KRouterLink
v-if="topics && topics.length"
ref="tab_button"
:to="foldersLink"
<HeaderTabs>
<HeaderTab
:text="coreString('folders')"
appearance="flat-button"
class="tab-button"
:style="!searchActive ? {
color: `${this.$themeTokens.primary} !important`,
borderBottom: `2px solid ${this.$themeTokens.primary}`,
paddingBottom: '2px',
} : {}"
:appearanceOverrides="customTabButtonOverrides"
:to="foldersLink"
/>
<KRouterLink
ref="tab_button"
:to="searchLink"
<HeaderTab
:text="coreString('searchLabel')"
appearance="flat-button"
class="tab-button"
:style="searchActive ? {
color: `${this.$themeTokens.primary} !important`,
borderBottom: `2px solid ${this.$themeTokens.primary}`,
paddingBottom: '2px',
} : {}"
:appearanceOverrides="customTabButtonOverrides"
:to="searchLink"
/>
</div>
</HeaderTabs>
</div>
<!-- mobile tabs (different alignment and interactions) -->
<div v-if="windowIsSmall" class="mobile-header">
<div class="mobile-header-contents">
<div class="mobile-tabs">
<KRouterLink
ref="tab_button"
:to="foldersLink"
<HeaderTabs>
<HeaderTab
:text="coreString('folders')"
appearance="flat-button"
:appearanceOverrides="customTabButtonOverrides"
:to="foldersLink"
/>
<KRouterLink
ref="tab_button"
:to="searchLink"
<HeaderTab
:text="coreString('searchLabel')"
appearance="flat-button"
:appearanceOverrides="customTabButtonOverrides"
:to="searchLink"
/>
</div>
</HeaderTabs>
<img
:src="topic.thumbnail"
class="channel-logo"
Expand Down Expand Up @@ -371,6 +346,8 @@
import { normalizeContentNode } from '../modules/coreLearn/utils.js';
import useSearch from '../composables/useSearch';
import genContentLink from '../utils/genContentLink';
import HeaderTabs from '../../../../coach/assets/src/views/common/HeaderTabs';
import HeaderTab from '../../../../coach/assets/src/views/common/HeaderTabs/HeaderTab';
Comment on lines +349 to +350
Copy link
Contributor

@indirectlylit indirectlylit Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Glad this was a straight-forward drop-in replacement!

Preferably we'd move this to core API spec (to avoid cross-plugin references) or even KDS (to make this component fully documented and reusable across products)

I'll let you and @rtibbles decide whether that should happen now or later

Copy link
Member

@rtibbles rtibbles Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm going to vote for later. Thanks to our updated string machinery, cross plugin imports like this are less of a concern, but the long import paths are not great.

Also good to think about the contrast between code used in common between apps vs things we want to expose as a common API.

I think most of our core API spec is the former at the moment, whereas it should be exclusively the latter.

import HybridLearningCardGrid from './HybridLearningCardGrid';
import EmbeddedSidePanel from './EmbeddedSidePanel';
import BrowseResourceMetadata from './BrowseResourceMetadata';
Expand Down Expand Up @@ -410,6 +387,8 @@
BrowseResourceMetadata,
SearchChips,
TextTruncator,
HeaderTab,
HeaderTabs,
},
mixins: [responsiveWindowMixin, commonCoreStrings],
setup() {
Expand Down Expand Up @@ -577,18 +556,6 @@
}
return false;
},
customTabButtonOverrides() {
return {
textTransform: 'capitalize',
paddingBottom: '10px',
fontWeight: 'normal',
':hover': {
color: this.$themeTokens.primary,
'background-color': this.$themeTokens.surface,
borderBottom: `2px solid ${this.$themeTokens.primary}`,
},
};
},
sidePanelWidth() {
if (this.windowIsSmall || (this.windowIsMedium && this.searchActive)) {
return 0;
Expand Down Expand Up @@ -775,14 +742,10 @@
margin: 12px;
}

.tabs {
.tab-block {
position: absolute;
bottom: 0;
}

.tab-button {
padding: 18px;
border-bottom: 2px solid transparent;
margin-bottom: 0;
}

.main-content-grid {
Expand Down Expand Up @@ -828,15 +791,10 @@

.mobile-header {
position: relative;
height: 100px;
height: 120px;
background-color: white;
}

.mobile-tabs {
position: absolute;
bottom: 0;
}

.channel-logo {
position: absolute;
top: 24px;
Expand Down