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

Convert titles and headers to VueRouter #4663

Merged
merged 16 commits into from
Aug 23, 2018

Conversation

OmgImAlexis
Copy link
Collaborator

Sorry for the single commit.

computed: {
isRoute() {
const { matched } = this.$route;
return matched.filter(route => route.components.default !== undefined).length >= 1;
Copy link
Contributor

@sharkykh sharkykh Jul 13, 2018

Choose a reason for hiding this comment

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

What's this property for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's for the future. It returns true if we have a matched component vs a matched route.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the home "partials"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No for determining if you're in a Vue component or a mako template.

@sharkykh
Copy link
Contributor

sharkykh commented Jul 13, 2018

  • /home
  • /home/displayShow
  • /home/snatchSelection
  • /home/editShow
  • /home/testRename
  • /home/postprocess
  • /home/status
  • /home/restart
  • /home/shutdown
  • /home/update - This route is too dynamic, can't add it right now
  • /login
  • /config
  • /config/anime
  • /config/backuprestore
  • /config/general
  • /config/notifications
  • /config/postProcessing
  • /config/providers
  • /config/search
  • /config/subtitles
  • /addShows
  • /addShows/newShow
  • /addShows/existingShow
  • /addShows/trendingShows
  • /addShows/popularShows
  • /addShows/popularAnime
  • /addRecommended
  • /schedule
  • /history
  • /manage
  • /manage/backlogOverview
  • /manage/episodeStatuses
  • /manage/failedDownloads
  • /manage/manageSearches
  • /manage/massEdit
  • /manage/subtitleMissed
  • /manage/subtitleMissedPP
  • /errorlogs
  • /errorlogs/viewlog
  • /news
  • /changes
  • /IRC
  • /not-found
  • /server-error - Maybe not needed at all?
  • /apibuilder - Not using layouts/main.mako

@OmgImAlexis

This comment has been minimized.

@sharkykh

This comment has been minimized.

@sharkykh

This comment has been minimized.

@OmgImAlexis

This comment has been minimized.

@OmgImAlexis

This comment has been minimized.

@sharkykh

This comment has been minimized.

@sharkykh

This comment has been minimized.

@OmgImAlexis

This comment has been minimized.

@OmgImAlexis OmgImAlexis modified the milestones: 0.2.7, 0.2.8 Jul 21, 2018
@sharkykh

This comment has been minimized.

@sharkykh sharkykh mentioned this pull request Jul 24, 2018
4 tasks
@sharkykh

This comment has been minimized.

@sharkykh sharkykh force-pushed the feature/vueify-title-and-header branch from 770956b to 4e2a350 Compare August 6, 2018 15:54
@sharkykh
Copy link
Contributor

sharkykh commented Aug 6, 2018

I recreated the branch on top of current develop, because we had so many changes (to develop),
so I went over the files and manually made the same changes this PR had before.
Hopefully I didn't make a mistake. A quick glance at the pages, especially help & info, home, displayShow and editShow, looks like everything is still good.

computed: {
isRoute() {
const { matched } = this.$route;
return matched.filter(route => route.components.default !== undefined).length >= 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately I still don't really understand why it's needed.
If you go to /home/displayShow it shouldn't load the home component.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It can be removed now. I was going to load all the pages differently than how I am now.

@sharkykh sharkykh force-pushed the feature/vueify-title-and-header branch from d2049a0 to 6079d37 Compare August 21, 2018 12:14
@sharkykh sharkykh changed the title convert most title and headers to vue router Convert titles and headers to VueRouter Aug 21, 2018
@sharkykh sharkykh mentioned this pull request Aug 21, 2018
24 tasks
@sharkykh sharkykh force-pushed the feature/vueify-title-and-header branch from 56c6784 to c636316 Compare August 22, 2018 06:30
@@ -4,12 +4,24 @@
from medusa import classes
%>
<%block name="scripts">

<%
if logLevel == logger.WARNING:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is this not available via URL params?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, yes, I can use that instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just gets us closer to SFC.

@sharkykh
Copy link
Contributor

sharkykh commented Aug 22, 2018

The only thing that really bugs me about this PR is the extra router-view component it adds for nested routes.
Maybe we should just drop the children as it seems to be intended for actual nested views...

image

@OmgImAlexis
Copy link
Collaborator Author

@sharkykh if you can find a way to do nested routes without it then by all means suggest away. The way I've used is what has been recommended over and over by the devs.

@sharkykh sharkykh force-pushed the feature/vueify-title-and-header branch from 13f5c18 to f827b36 Compare August 23, 2018 07:10
@sharkykh
Copy link
Contributor

@OmgImAlexis Last commit 'fixes' it.
I'm sure we can also find a way to not write the path prefix for each route group (/home/, /config/), if you don't like it.

They don't have paths so nothing to add to the router.
import router from '../../src/router';

test('router compiles', t => {
t.truthy(router);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think notThrows would work better here.

Copy link
Contributor

Choose a reason for hiding this comment

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

notThrows requires a function, but the import happens at the top of the file.

Copy link
Collaborator Author

@OmgImAlexis OmgImAlexis left a comment

Choose a reason for hiding this comment

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

LGTM

@OmgImAlexis OmgImAlexis merged commit 7cb7584 into develop Aug 23, 2018
@OmgImAlexis OmgImAlexis deleted the feature/vueify-title-and-header branch August 23, 2018 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants