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

fix: add summary to partner show pages #462

Open
wants to merge 2 commits into
base: staging
Choose a base branch
from

Conversation

kimadactyl
Copy link
Member

@kimadactyl kimadactyl commented Nov 20, 2024

Fixes #433

Description

Shows summary and description on show pages.

Note that this bug existing has resulted in people improvising in the backend repeating info - example below. We may need to do some post-fix cleanup.

Screenshot 2024-11-20 at 16 24 23

Screenshot showing change vs original bug ticket:

Screenshot 2024-11-20 at 16 29 39

Huly®: TD-458

Copy link

cloudflare-workers-and-pages bot commented Nov 20, 2024

Deploying the-trans-dimension with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8b13122
Status: ✅  Deploy successful!
Preview URL: https://5b31ce43.the-trans-dimension.pages.dev
Branch Preview URL: https://add-summary-to-partner-show.the-trans-dimension.pages.dev

View logs

@kimadactyl
Copy link
Member Author

A bit out my depth here but thought I'd give it a crack!

@katjam katjam changed the base branch from main to staging November 24, 2024 13:09
Copy link
Member

@katjam katjam left a comment

Choose a reason for hiding this comment

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

As with everything, this is a bit more complicated than it first appears, but good effort. Apart from the repeating empty test, this would appear fine to end user.

@@ -2,7 +2,7 @@ module Theme.Page.Partner exposing (viewInfo)

import Copy.Keys exposing (Key(..))
import Copy.Text exposing (t)
import Css exposing (Style, auto, batch, calc, center, color, displayFlex, fontStyle, important, margin2, margin4, marginBlockEnd, marginBlockStart, marginTop, maxWidth, minus, normal, pct, px, rem, textAlign, width)
import Css exposing (Style, auto, batch, calc, center, color, displayFlex, fontStyle, important, margin2, margin4, marginBlockEnd, marginBlockStart, marginTop, maxWidth, minus, normal, pct, px, rem, textAlign, text_, width)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you ended up using this text_ import

Copy link
Member Author

Choose a reason for hiding this comment

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

oh i think vscode must have inserted this automatically! annoying it adds and doesn't cut

, div [ css [ descriptionStyle ] ]
(Theme.TransMarkdown.markdownToHtml
(t (PartnerDescriptionText partner.summary partner.name)
++ "\n\n"
Copy link
Member

Choose a reason for hiding this comment

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

I think instead of the manual newline between the 2 sections of text, we could copy the whole thing and make 2 divs - depends what styles we want to apply to both but we might want the descriptionStyle css to wrap both.

We also probably only want the "Ask partner for more information" to show for one or other. I guess leave it on the description? I think maybe this site was designed with the summary being used in the list and the description being used in this page so we might even want more logic to check if they repeat each other or only show empty text if both are empty?

It would be more clear if that logic to show empty text was here instead of in the translation file.

Try something like:

, div [ css [ descriptionStyle ] ] 
    [ div [ ] (Theme.TransMarkdown.markdownToHtml partner.summary)
    , div [ ] (Theme.TransMarkdown.markdownToHtml (t (PartnerDescriptionText partner.description partner.name)))
    ]

or

, div [ css [ descriptionStyle ] ] (Theme.TransMarkdown.markdownToHtml partner.summary)
, div [ css [ descriptionStyle ] ] (Theme.TransMarkdown.markdownToHtml (t (PartnerDescriptionText partner.description partner.name)))

Copy link
Member Author

@kimadactyl kimadactyl Nov 24, 2024

Choose a reason for hiding this comment

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

i tried this first but it adds awkward looking spacing between bc the div has padding on it too - i don't think we want the extra div? (reminder the markdown converter puts the summary in a p tag automatically)

ah i missed the no text state - i guess ideally this would be a fuction that processes the whole thing but getting a bit out my depth! i'll give it a go next week

Copy link
Member

Choose a reason for hiding this comment

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

We can fix the styling on the divs if there is too much padding - maybe we need to make a new style and not use the existing descriptionStyle.

You're doing fine with it! And yes, I think I'm inclined to change that copy key to PartnerDecriptionEmptyText and then as you say, make a function here that only shows it if both summary & description are empty. Shout if you want a hand.

@kimadactyl kimadactyl self-assigned this Nov 25, 2024
@kimadactyl
Copy link
Member Author

hey im struggling mentally with the lottery thing and think this might be one thing too many for my brain rn, if youre happy to finish this off please do i think i just need to go not be ok for a bit

@kimadactyl kimadactyl removed their assignment Nov 27, 2024
@katjam
Copy link
Member

katjam commented Nov 28, 2024

Of course. No problem. Look after yourself.

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.

[Bug]: Partner pages not showing partner summaries
2 participants