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

Added option for horizontal table of contents #265

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

KashishMistry
Copy link
Member

@KashishMistry KashishMistry commented Mar 14, 2024

Related Item(s)

ramp4-pcar4/story-ramp#402 (not in this repo)

Changes

  • Added option for user to select between horizontal and vertical table of contents
  • The default option is a vertical table of contents

Notes

Option in the metadata panel:
toc orientation

Tested in editor preview by creating a local story-ramp package:
horizontaltabsinpreview

Testing

Steps:

  1. Open Project Metadata editor. Choose between horizontal or vertical table of contents.

This has been tested in the editor preview using by creating a local story-ramp package based on the adjacent PR.

There isn't a way to test whether the functionality works because there is another PR open in the story-ramp repo that is meant to implement the functionality. If there are any other methods to test, that would be appreciated, thanks!


This change is Reviewable

Copy link

Your demo site is ready! 🚀 Visit it here: https://ramp4-pcar4.github.io/storylines-editor/horizontal_tabs/#/en/editor

Copy link
Member

@RyanCoulsonCA RyanCoulsonCA left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @KashishMistry)


src/components/editor/helpers/metadata-content.vue line 75 at r1 (raw file):

        <br />
        <label class="mr-15">{{ $t('editor.tocOrientation') }}:</label>
        <select class="border-solid border border-black p-1" name="toc" id="toc" @change="metadataChanged">

Tested this by running the server locally and checking the config manually after saving. Found a couple of things:

  1. It doesn't seem like the orientation is being saved to the file. Changing the name attribute to tocOrientation should fix this.

  2. The dropdown also doesn't default to the correct value. Try adding v-model="metadata.tocOrientation" here and add tocOrientation to the metadata prop (see other comment) and see if this works!


src/components/editor/helpers/metadata-content.vue line 95 at r1 (raw file):

export default class MetadataEditorV extends Vue {
    @Prop() metadata!: {

Add tocOrientation here

Copy link
Member Author

@KashishMistry KashishMistry left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @RyanCoulsonCA)


src/components/editor/helpers/metadata-content.vue line 75 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Tested this by running the server locally and checking the config manually after saving. Found a couple of things:

  1. It doesn't seem like the orientation is being saved to the file. Changing the name attribute to tocOrientation should fix this.

  2. The dropdown also doesn't default to the correct value. Try adding v-model="metadata.tocOrientation" here and add tocOrientation to the metadata prop (see other comment) and see if this works!

Donethanks.


src/components/editor/helpers/metadata-content.vue line 95 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Add tocOrientation here

Donethanks.

Copy link
Member

@RyanCoulsonCA RyanCoulsonCA left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @KashishMistry)


src/components/editor/helpers/metadata-content.vue line 75 at r1 (raw file):

Previously, KashishMistry (Kashish Mistry) wrote…

Donethanks.

Looks good! If you load an existing product that doesn't have tocOrientation in the config the dropdown is blank, but I'm not sure that this is a huge deal as long as we display the vertical table of contents by default.

Copy link
Member

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @KashishMistry)

Copy link
Member Author

@KashishMistry KashishMistry left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA)


src/components/editor/helpers/metadata-content.vue line 75 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Looks good! If you load an existing product that doesn't have tocOrientation in the config the dropdown is blank, but I'm not sure that this is a huge deal as long as we display the vertical table of contents by default.

Yes, it should default to vertical if there is no tocOrientation specified

Copy link
Member

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA)

Copy link
Member

@szczz szczz left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 5 of 5 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA)

Copy link
Member

@RyanCoulsonCA RyanCoulsonCA left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 3 of 5 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @KashishMistry)

Copy link
Member

@yileifeng yileifeng left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @KashishMistry)

@yileifeng yileifeng merged commit a2294fb into ramp4-pcar4:main Apr 4, 2024
3 checks passed
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.

4 participants