Enhancement - #6175: Added option for responsive Paragraph and Text text #6451
Conversation
…nd grouped sizes in an object
…existing method in the Heading component
…ied responsive breakpoint
|
Just wanted to give a quick update on the review status of this pull request. I have been looking into it, the problem is changing the theme structure to include |
|
Hey @georgiabains thanks for your patience on this! I chatted with some other members of the team this morning and we came to a solution. We are thinking instead of changing the theme for paragraph and text to have fontSize we could change The responsive breakpoint object would look something like this for Text and Paragraph: text: {
...
responsiveBreakpoint: {
small: { // here small refers to the screen size breakpoint
small: 'xsmall', // here small refers to text size small and says when we are in a small breakpoint and the text size is small use the xsmall size defined by text.xsmall in the theme
medium: 'small',
large: 'medium',
},
medium: {
...
}
...
}For backwards compatibility Let me know if this makes sense or if there are any questions. I appreciate your work on this and happy to help out or clarify in any way needed! |
|
Hey @jcfilben thanks for responding! That solution makes way more sense rather than forcing countless users to update their codebases 😅 I can work on making this update this week 👍 |
Perfect! |
jcfilben
added
needs attention
What does this PR do?
This PR mainly modifies the Paragraph and Text schemas and components to add a
responsiveBreakpointthat is used to scale down font sizes for resolutions under that breakpoint. The method follows that ofHeadingquite closely, however I did have to change the schema to group the font sizes within one object, in order to create an ordered array to loop through to determine the appropriate smaller size.Where should the reviewer start?
Looking at
base.js,StyledParagraph.jsandStyledText.js.What testing has been done on this PR?
I ran
yarn test-updateandyarn testbefore each commit and created Paragraph and Text components in my local host, set up withresponsive={true}and tested the different possiblesizes. I also tried inputtingsizes that didn't exist, to confirm that text still displayed.How should this be manually tested?
In a localhost, set up a Paragraph and Text component with the property
responsive={true}. Confirm that resolutions under 768px have a smaller font size. Further testing can be done by changing thesizeproperty.Do Jest tests follow these best practices?
N/A
screenis used for querying.userEventis used in place offireEvent.asFragment()is used for snapshot testing.Any background context you want to provide?
I followed the existing coding structure that the Heading component uses.
What are the relevant issues?
Link to github issue (6175)
Screenshots (if appropriate)
N/A
Do the grommet docs need to be updated?
No
Should this PR be mentioned in the release notes?
I'm afraid I might be a little unfamiliar with Grommet, do users have the ability to directly target theme props? If so then it should be, so that users can update any references to
props.theme.paragraphandprops.theme.text.Is this change backwards compatible or is it a breaking change?
I believe its a breaking change, as modifying the schema for Text resulted in modifying references to
props.theme.text, which are used in other components.