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 inline description list variant #3439

Merged

Conversation

precious-onyenaucheya-ons
Copy link
Contributor

@precious-onyenaucheya-ons precious-onyenaucheya-ons commented Nov 27, 2024

What is the context of this PR?

ONSDESYS-189....

Created new Description list variant as per the Figma.

Prototype link

How to review this PR

Check that new example-inline-description-list in Description List component matches the figma design

Checklist

This needs to be completed by the person raising the PR.

  • I have selected the correct Assignee
  • I have linked the correct Issue

@precious-onyenaucheya-ons precious-onyenaucheya-ons added the Enhancement Change of existing feature label Nov 27, 2024
@precious-onyenaucheya-ons precious-onyenaucheya-ons requested a review from a team November 27, 2024 12:48
Copy link

netlify bot commented Nov 27, 2024

Deploy Preview for ons-design-system-preview ready!

Name Link
🔨 Latest commit 8f45296
🔍 Latest deploy log https://app.netlify.com/sites/ons-design-system-preview/deploys/6756e33b13689c000946f7bd
😎 Deploy Preview https://deploy-preview-3439--ons-design-system-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rmccar
Copy link
Contributor

rmccar commented Nov 28, 2024

Looks like its possible for the colon to wrap onto another line

Screenshot 2024-11-28 at 10 46 19

@rmccar
Copy link
Contributor

rmccar commented Nov 28, 2024

I think we need to check on how this behaves with smaller screens with Joe, the figma still has it behave like this
Screenshot 2024-11-28 at 11 52 27
but the document list that we have at the moment behaves like this
Screenshot 2024-11-28 at 11 53 09
We may need to adjust the breakpoints

@admilne
Copy link
Contributor

admilne commented Nov 28, 2024

I think the use of dt and dd tags are breaking the style from the prototype. At the moment this is forcing the text to wrap in a way different to the prototype. Look at the 'Business' column in this example.

image

This is the example in the prototype. You can see from the code that they are using a span tag for the 'Business' column and then just test for information about the release - so the text will wrap underneath 'Business'

Screenshot 2024-11-28 123351

@precious-onyenaucheya-ons
Copy link
Contributor Author

I think we need to check on how this behaves with smaller screens with Joe, the figma still has it behave like this Screenshot 2024-11-28 at 11 52 27 but the document list that we have at the moment behaves like this Screenshot 2024-11-28 at 11 53 09 We may need to adjust the breakpoints

I confirmed from Joe and I have updated it to fit with the figma

@precious-onyenaucheya-ons precious-onyenaucheya-ons changed the title Add horizontal description list variant Add inline description list variant Dec 5, 2024
@rmccar
Copy link
Contributor

rmccar commented Dec 5, 2024

This is the prototype with a screen width of about 950px:

Screenshot 2024-12-05 at 15 18 12



This is your branch with a screen width of about 950px:

Screenshot 2024-12-05 at 15 19 42

We need to check this is correct because it isn't quite the same at the prototype

@rmccar
Copy link
Contributor

rmccar commented Dec 6, 2024

This change to columns is now causing this issue

Screenshot 2024-12-06 at 09 23 45

@rmccar
Copy link
Contributor

rmccar commented Dec 6, 2024

I think you need to revert the col change otherwise we are getting this. And in this case I think the wrapping is probably the better of the two so we should remove white-space: nowrap;

Screenshot 2024-12-06 at 14 51 08

@precious-onyenaucheya-ons precious-onyenaucheya-ons merged commit 2f6bcf0 into main Dec 9, 2024
25 checks passed
@precious-onyenaucheya-ons precious-onyenaucheya-ons deleted the feature/189/add-horizontal-description-list branch December 9, 2024 13:29
@SriHV SriHV mentioned this pull request Jan 13, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Change of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants