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

add preview #85

Merged
merged 6 commits into from
Jun 21, 2024
Merged

add preview #85

merged 6 commits into from
Jun 21, 2024

Conversation

VortexExpansion
Copy link
Contributor

@VortexExpansion VortexExpansion commented Jun 3, 2024

Fixes #84

Checkpoints

@VortexExpansion
Copy link
Contributor Author

@yabe-diverta san
Preview example : https://diverta.gyazo.com/eeff07c4230c40e08b376e491c6ad29e

I had to update the common default.vue in layouts.
Please review following points and confirm if them are OK/what could be the alternatives :

  • I force check if preview_token is not defined and then hide/unhide the elements
  • The items in the left navbar are black in preview page (different from normal page) because I removed the "to" link from them which was adding the white text styling.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would have better to provide gray text instead of back one due to looking invisible a bit so could you provide a special class for it?

Copy link
Contributor Author

@VortexExpansion VortexExpansion Jun 5, 2024

Choose a reason for hiding this comment

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

@yabe-diverta san
I only know inline styles like this :style="{ color: preview_token ? 'gray' : 'white' }.
Not very experienced in styles.
And cannot find the class for the "to" link in the scss files.

Can you please suggest where to change ?

Copy link
Contributor

@yabe-diverta yabe-diverta Jun 6, 2024

Choose a reason for hiding this comment

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

@VortexExpansion
I pushed a new commit to set it, could you confirm it?
https://diverta.gyazo.com/16570bad97de39c68b41d21cb8c39140
063aba9

@VortexExpansion
Copy link
Contributor Author

@HisashiHasebe san
Only preview for article details is sufficient or do we need preview for some more pages ?

@HisashiHasebe
Copy link
Contributor

@VortexExpansion
I think only for article details is enough.

@VortexExpansion VortexExpansion marked this pull request as ready for review June 6, 2024 03:16
Copy link
Contributor

@yabe-diverta yabe-diverta Jun 6, 2024

Choose a reason for hiding this comment

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

@VortexExpansion
I pushed a new commit to set it, could you confirm it?
https://diverta.gyazo.com/16570bad97de39c68b41d21cb8c39140
063aba9

@VortexExpansion
Copy link
Contributor Author

@yabe-diverta san

I had a suggestion.
Instead of extracting the preview token from the router in the layout and using that as a condition for rendering items selectively, I think we can just make a separate custom layout for preview page which only works for preview and does not worry about login status too.

While selectively choosing elements and class rendering I also have to take care of whether the user is logged in or not and that is adding to the complexity of conditional checks.

Separating the layout altogether solves the problem and elimates the need for extracting the preview token in the layout page.

Your thoughts on this ?

@yabe-diverta
Copy link
Contributor

yabe-diverta commented Jun 6, 2024

@VortexExpansion
I think it sounds good if we can wisely separate layout files.
A one thing I'm concerning is how can we manage not to have similar 2 layout files?
Not to have same-copy layout files.
For example along with your suggestion we could make a new layout file witch just passes a prop to existing layout file, although it still needs condition within the existing one.
Like props.isPreview ? 'previewClass' : null that is quite similar to your PR content.

@VortexExpansion
Copy link
Contributor Author

@yabe-diverta san
This is the basic change that I did. Please check till this point.

Further, I think the only things that can be taken out is NavDrawer component something like this
/components/NavDrawer.vue

<template>
    <v-navigation-drawer id="c-navi_side" v-model="model" left fixed dark permanent >
        <div class="text-center py-4">
            <a href="/">
                <img src="~/assets/images/logo.png?width=150" class="c-navi_side-logo" />
            </a>
        </div>
        <v-list>
            <v-list-item v-for="(item, i) in items" :key="i" :to="item.to" router class="l-mainmenu_item" :class="{ disabled: item.disabled }">
                <v-list-item-action>
                    <v-icon>{{ item.icon }}</v-icon>
                </v-list-item-action>
                <v-list-item-title v-text="item.title" />
            </v-list-item>
        </v-list>
    </v-navigation-drawer>
</template>

<script setup>
const model = defineModel();
const props = defineProps({
    items: {
        type: Array,
        required: false,
        default: () => []
    },
});
</script>

and pass props like

const navItems = useNavDrawerItems().map(item => ({ ...item, disabled: true }));

from preview.vue

I tried but this is failing the drawer interactive button due to v-model pass issue.
Documentation for passing v-model as props : https://vuejs.org/guide/components/v-model.html#usage-with-v-model

Even though I integrated it, it doesn't seem to work. Please check if you have time or leave a comment if it is OK up to this point.

@VortexExpansion
Copy link
Contributor Author

@yabe-diverta san
Also on another note, the drawer seems to close on full screen automatically even though its display is set to permanent.
Can't seem to figure out why.
https://diverta.gyazo.com/bea237d2efd1acef2bb3f6ee226b1166

@VortexExpansion
Copy link
Contributor Author

@yabe-diverta san
Please check.
Nav drawer fixed now. https://diverta.gyazo.com/85c47ca0626428c742400b8a065c489d

<v-app class="l-content_wrap">
<v-navigation-drawer id="c-navi_side" left fixed dark permanent>
<v-app class="l-content_wrap p-dashboard">
<v-navigation-drawer id="c-navi_side" v-model="drawer" left fixed dark permanent>
Copy link
Contributor

Choose a reason for hiding this comment

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

oh good solution

@VortexExpansion
Copy link
Contributor Author

@HisashiHasebe san
Please review and merge

@HisashiHasebe HisashiHasebe merged commit e4b2461 into diverta:main Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preview support
3 participants