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

1889 As editors we see a need to have text promotion cards to link to other pages #1943

Closed
wants to merge 22 commits into from

Conversation

millianapia
Copy link
Contributor

No description provided.

@millianapia millianapia requested a review from a team as a code owner October 20, 2023 09:50
Copy link
Contributor

@fernandolucchesi fernandolucchesi left a comment

Choose a reason for hiding this comment

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

Left some comments

sanityv3/schemas/objects/promoTextTile.tsx Outdated Show resolved Hide resolved
sanityv3/schemas/objects/promoTextTile.tsx Outdated Show resolved Hide resolved
sanityv3/schemas/objects/promoTextTile.tsx Outdated Show resolved Hide resolved
sanityv3/schemas/objects/promoTextTile.tsx Outdated Show resolved Hide resolved
web/lib/queries/common/pageContentFields.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@fernandolucchesi fernandolucchesi left a comment

Choose a reason for hiding this comment

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

Few comments:

  1. title vs text content. Do we really need 2 fields? And it looks like only title is being used:
image image
  1. without space between components: space is there, what does it mean to be without? image with space between components: space is applied to the cards, in which doesn't make sense to me specially on mobile view.
image Is this the expected behaviour? (I haven't checked how the other component cited by Nick works)
  1. Design issues: Text is too small. Inner paddings of cards are too small.
  2. Feature is missing "11. Consider component to be hidden under access control"
  3. Feature is missing "13. Should be possible to create with option not to have a link"
  4. Not in the specification but I'd assume it has to be done: text should be always aligned left on mobile view

@millianapia
Copy link
Contributor Author

millianapia commented Nov 9, 2023

  • Feature is missing "11. Consider component to be hidden under access control"

this will be done later, not now. As discussed with @NickHaggerty1

Copy link
Contributor

@fernandolucchesi fernandolucchesi left a comment

Choose a reason for hiding this comment

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

  1. Text is not centered:
    image

  2. Text does not fit component properly:
    image

  3. Link is not showing up?
    image
    image

  4. This description is a bit confusing: '
    Toggle button text
    This will add the text to link'. I did not understand what this does, and assume editors will also get confused.

  5. Component name is Promo tiles array, I think Promo text tiles would be more descriptive for editors

Copy link
Contributor

@fernandolucchesi fernandolucchesi left a comment

Choose a reason for hiding this comment

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

the component is still not looking as it should, label is not being displayed, arrow is on the center when text is to the left, and a lot of empty space between text and arrow:
image

Make sure to test all functionalities / screen resolutions and use cases when working on these kind of features. I recommend dragging the window to resize it instead of testing in fixed res, to ensure it looks good regardless of the screen resolution

sanityv3/schemas/objects/promoTextTile.tsx Outdated Show resolved Hide resolved
sanityv3/schemas/objects/promoTextTile.tsx Outdated Show resolved Hide resolved
sanityv3/schemas/objects/promoTextTileArray.tsx Outdated Show resolved Hide resolved
sanityv3/schemas/objects/promoTextTileArray.tsx Outdated Show resolved Hide resolved
sanityv3/schemas/objects/promoTextTile.tsx Show resolved Hide resolved
web/lib/queries/promoTextTileArrayFields.ts Outdated Show resolved Hide resolved
sanityv3/schemas/objects/promoTextTile.tsx Outdated Show resolved Hide resolved
@fernandolucchesi
Copy link
Contributor

I haven't reviewed the css part yet since the component is not working as it should

Copy link
Contributor

@fernandolucchesi fernandolucchesi left a comment

Choose a reason for hiding this comment

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

Doesn't work for big screens, grid is not limited at 2:

image

image

Copy link
Contributor

@fernandolucchesi fernandolucchesi left a comment

Choose a reason for hiding this comment

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

I don't want to be super picky all the time, so I suggest checking with Nick if the way this is implemented is good enough. But if we are to follow the design suggested, there are a few discrepancies:

  1. Spacing between components are too big IMO even when not using 'additional space':
    image
    vs

image

  1. Action (arrow) on mobile should be left aligned, not center aligned:
    image

  2. Line break doesn't work

  3. I honestly don't see why we should limit this to 100 characters, or limit it at all. It is too few IMO, we can't even replicate the example card:
    image

  4. Spacing between text and action is a bit strange when comparing to the design
    image

  5. If it is a single card, text should be centered, it is aligned to left:

image vs
image

@millianapia millianapia deleted the malj/1889 branch December 20, 2023 14:04
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.

2 participants