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

Create caption input for maps and charts #428

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

IshavSohal
Copy link
Member

@IshavSohal IshavSohal commented Nov 20, 2024

Related Item(s)

#393

Changes

  • Add caption input for maps and maps
  • Replace hardcoded 'Done' with translated text

Notes

ramp4-pcar4/story-ramp#505 handles the map caption in the StoryRAMP repo. ramp4-pcar4/story-ramp#507 handles the chart caption.

Testing

Steps:

  1. Load in a product
  2. Open a slide that contains a map (or create one)
  3. Within the panel containing the map, observe the input field for the caption above the map
  4. Enter a caption and save changes
  5. Within the product's config, observe that the map has a caption field with the caption you provided
  6. Open a slide that contains a chart (or create one)
  7. Within the panel containing the chart, observe the input field for the caption below the chart
  8. Enter a caption and save changes
  9. Within the product's config, observe that the chart has a caption field with the caption you provided

This change is Reviewable

@IshavSohal IshavSohal added the PR: Frontend PR that primarily involves frontend changes. UI experts and CSS Wizards are asked to review. label Nov 20, 2024
Copy link

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

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 8 files reviewed, 1 unresolved discussion (waiting on @IshavSohal)


src/components/helpers/chart-preview.vue line 67 at r1 (raw file):

        </div>
        <div class="flex mt-4">
            <label :for="'chartPreviewCaption' + index" class="font-bold"

There seems to be an alignment issue with the label and the input box. We should center align the text with the input for consistency.
image.png

@IshavSohal IshavSohal force-pushed the issue-393 branch 2 times, most recently from a0aed09 to 4132b98 Compare November 26, 2024 21:15
Copy link
Member Author

@IshavSohal IshavSohal 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 8 files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA)


src/components/helpers/chart-preview.vue line 67 at r1 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

There seems to be an alignment issue with the label and the input box. We should center align the text with the input for consistency.
image.png

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.

Reviewed 3 of 8 files at r1, 1 of 5 files at r2, all commit messages.
Reviewable status: 4 of 8 files reviewed, 3 unresolved discussions (waiting on @IshavSohal)


src/components/map-editor.vue line 36 at r2 (raw file):

                    type="text"
                    v-model="panel.caption"
                    placeholder="Add a caption"

This will need a translation!


src/components/helpers/chart-preview.vue line 75 at r2 (raw file):

                type="text"
                v-model="chart.caption"
                placeholder="Add a caption"

Same here, needs translation

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.

image.png

I think vertically aligning the caption input with the text of the chart name would look nicer for the UI

Reviewed 3 of 8 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @IshavSohal and @RyanCoulsonCA)

Copy link
Member

@spencerwahl spencerwahl 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 8 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @IshavSohal and @RyanCoulsonCA)

@IshavSohal IshavSohal force-pushed the issue-393 branch 2 times, most recently from adfcdf1 to 19f234e Compare December 3, 2024 20:38
Copy link
Member Author

@IshavSohal IshavSohal left a comment

Choose a reason for hiding this comment

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

I was able to get this working, but things get a little crowded for smaller screen widths. Let me know if you prefer the way it used to look, or if you have any suggestions.

Reviewable status: 5 of 8 files reviewed, 3 unresolved discussions (waiting on @RyanCoulsonCA, @spencerwahl, and @yileifeng)


src/components/map-editor.vue line 36 at r2 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

This will need a translation!

Donethanks


src/components/helpers/chart-preview.vue line 75 at r2 (raw file):

Previously, RyanCoulsonCA (Ryan Coulson) wrote…

Same here, needs translation

Donethanks

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.

I like the new look, but looks like the styling for chart caption input was lost in the latest changes?

image.png

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @RyanCoulsonCA)

Copy link
Member Author

@IshavSohal IshavSohal left a comment

Choose a reason for hiding this comment

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

I initially reduced the text input's height to keep it in line with the 'caption:', but it should be good with the original height now

Reviewable status: 7 of 8 files reviewed, 3 unresolved discussions (waiting on @RyanCoulsonCA and @yileifeng)

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 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (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 1 of 5 files at r2, 2 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @IshavSohal)

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 8 files at r1, 2 of 5 files at r2, 2 of 3 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RyanCoulsonCA)

@yileifeng yileifeng merged commit 4788db1 into ramp4-pcar4:main Dec 13, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Frontend PR that primarily involves frontend changes. UI experts and CSS Wizards are asked to review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants