-
Notifications
You must be signed in to change notification settings - Fork 9
[OSDEV-2368] Add reusable labels, default value fallback, and external link icons to partner fields #912
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
Open
protsack-stephan
wants to merge
7
commits into
main
Choose a base branch
from
OSDEV-2368-partner-field-enhancements
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
[OSDEV-2368] Add reusable labels, default value fallback, and external link icons to partner fields #912
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
18cdfb5
Update the util that figures out the label
protsack-stephan 333ef5a
Extract PartnerFieldLabel component for partner field title styling
protsack-stephan 401a05f
Make label a separate component
protsack-stephan 9a5ff47
Add the ability to fallback to default value
protsack-stephan 37c5d06
[OSDEV-2368] Add external link icons and custom link text support to …
protsack-stephan eccac59
Merge latest main
protsack-stephan 4226000
Update DateProperty to avoid undefined error
protsack-stephan File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| --- | ||
| name: pr-description | ||
| description: Write high-quality pull request descriptions based on Google's CL description best practices. Use when creating a pull request, writing a PR description, or when the user asks to describe changes for a PR or merge request. | ||
| --- | ||
|
|
||
| # Writing Good PR Descriptions | ||
|
|
||
| Based on [Google's CL description guidelines](https://google.github.io/eng-practices/review/developer/cl-descriptions.html). | ||
|
|
||
| A PR description is a public record of change. It must communicate: | ||
|
|
||
| 1. **What** change is being made — summarize so readers understand without reading the entire diff. | ||
| 2. **Why** these changes are being made — what context did the author have? What decisions aren't reflected in the code? | ||
|
|
||
| The description becomes a permanent part of version control history. Future developers will search for the PR based on its description. If all the important information is in the code and not the description, it will be much harder to locate. And even after finding it, they need to understand *why* the change was made — code reveals what the software does, but not why it exists. | ||
|
|
||
| ## Gathering Context | ||
|
|
||
| Before writing the description, run these commands in parallel to understand the full scope of changes: | ||
|
|
||
| - `git log --oneline <base-branch>..HEAD` | ||
| - `git diff <base-branch>..HEAD --stat` | ||
| - `git diff <base-branch>..HEAD` | ||
| - `git log <base-branch>..HEAD --format="%B---"` | ||
|
|
||
| ## First Line | ||
|
|
||
| - Short summary of specifically **what** is being done. | ||
| - Complete sentence, written as though it were an order (imperative mood). | ||
| - Followed by an empty line. | ||
|
|
||
| The first line appears in version control history summaries, so it must be informative enough that future code searchers don't have to read the full description to understand what the PR did. It should stand alone, allowing readers to skim history quickly. | ||
|
|
||
| Keep it short, focused, and to the point. Clarity and utility to the reader is the top concern. | ||
|
|
||
| Say "**Delete** the FizzBuzz RPC and **replace** it with the new system." instead of "**Deleting** the FizzBuzz RPC and **replacing** it with the new system." | ||
|
|
||
| The rest of the description does not need to be imperative. | ||
|
|
||
| ## Body is Informative | ||
|
|
||
| The rest of the description should fill in the details and include any supplemental information a reader needs to understand the changelist holistically: | ||
|
|
||
| - A brief description of the problem being solved | ||
| - Why this is the best approach | ||
| - Any shortcomings to the approach | ||
| - Background information: bug/ticket numbers, benchmark results, links to design documents | ||
|
|
||
| If you include links to external resources, consider that they may not be visible to future readers due to access restrictions or retention policies. Where possible, include enough context for reviewers and future readers to understand the PR without following the links. | ||
|
|
||
| Even small PRs deserve attention to detail. Put the PR in context. | ||
|
|
||
| ## Bad PR Descriptions | ||
|
|
||
| "Fix bug" is an inadequate description. What bug? What did you do to fix it? Other bad examples: | ||
|
|
||
| - "Fix build." | ||
| - "Add patch." | ||
| - "Moving code from A to B." | ||
| - "Phase 1." | ||
| - "Add convenience functions." | ||
| - "kill weird URLs." | ||
|
|
||
| These do not provide enough useful information. | ||
|
|
||
| ## Good PR Descriptions | ||
|
|
||
| ### Functionality change | ||
|
|
||
| > RPC: Remove size limit on RPC server message freelist. | ||
| > | ||
| > Servers like FizzBuzz have very large messages and would benefit from reuse. Make the freelist larger, and add a goroutine that frees the freelist entries slowly over time, so that idle servers eventually release all freelist entries. | ||
|
|
||
| The first few words describe what the PR does. The rest talks about the problem being solved, why this is a good solution, and the specific implementation. | ||
|
|
||
| ### Refactoring | ||
|
|
||
| > Construct a Task with a TimeKeeper to use its TimeStr and Now methods. | ||
| > | ||
| > Add a Now method to Task, so the borglet() getter method can be removed (which was only used by OOMCandidate to call borglet's Now method). This replaces the methods on Borglet that delegate to a TimeKeeper. | ||
| > | ||
| > Allowing Tasks to supply Now is a step toward eliminating the dependency on Borglet. Eventually, collaborators that depend on getting Now from the Task should be changed to use a TimeKeeper directly, but this has been an accommodation to refactoring in small steps. | ||
| > | ||
| > Continuing the long-range goal of refactoring the Borglet Hierarchy. | ||
|
|
||
| The first line describes what the PR does and how this is a change from the past. The rest describes the specific implementation, the context, that the solution isn't ideal, and possible future direction. It explains *why* the change is being made. | ||
|
|
||
| ### Small PR that needs some context | ||
|
|
||
| > Create a Python3 build rule for status.py. | ||
| > | ||
| > This allows consumers who are already using this as in Python3 to depend on a rule that is next to the original status build rule instead of somewhere in their own tree. It encourages new consumers to use Python3 if they can, instead of Python2, and significantly simplifies some automated build file refactoring tools being worked on currently. | ||
protsack-stephan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| The first sentence describes what's being done. The rest explains *why* and gives the reviewer context. | ||
|
|
||
| ## Using Tags | ||
|
|
||
| Tags are manually entered labels that categorize PRs (e.g. `[tag]`, `#tag`, `tag:`). They are optional. | ||
|
|
||
| If using tags, consider whether they belong in the body or the first line. Limit tag usage in the first line so it doesn't obscure the content. | ||
|
|
||
| Good: | ||
| - `[banana] Peel the banana before eating.` | ||
| - `#banana #apple: Assemble a fruit basket.` | ||
|
|
||
| Bad: | ||
| - `[banana peeler factory factory][apple picking service] Assemble a fruit basket.` — too many/long tags overwhelm the first line. | ||
|
|
||
| ## Review Before Submitting | ||
|
|
||
| PRs can undergo significant change during review. Review the description before submitting to ensure it still reflects what the PR does. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| import React from 'react'; | ||
| import { render, screen } from '@testing-library/react'; | ||
| import moment from 'moment'; | ||
| import DateProperty from '../../components/PartnerFields/FormatComponents/DateProperty'; | ||
|
|
||
| const defaultProps = { | ||
| propertyKey: 'start_date', | ||
| value: { start_date: '2024-06-15' }, | ||
| schemaProperties: { start_date: {} }, | ||
| }; | ||
|
|
||
| describe('DateProperty', () => { | ||
| it('renders a formatted date from value', () => { | ||
| render(<DateProperty {...defaultProps} />); | ||
|
|
||
| const expected = moment('2024-06-15').format('LL'); | ||
| expect(screen.getByText(expected)).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('falls back to schema default date when value is missing', () => { | ||
| render( | ||
| <DateProperty | ||
| {...defaultProps} | ||
| value={{}} | ||
| schemaProperties={{ | ||
| start_date: { default: '2024-01-15' }, | ||
| }} | ||
| />, | ||
| ); | ||
|
|
||
| const expected = moment('2024-01-15').format('LL'); | ||
| expect(screen.getByText(expected)).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('displays the title from schema', () => { | ||
| render( | ||
| <DateProperty | ||
| {...defaultProps} | ||
| schemaProperties={{ | ||
| start_date: { title: 'Start Date' }, | ||
| }} | ||
| />, | ||
| ); | ||
|
|
||
| expect(screen.getByText(/Start Date:/)).toBeInTheDocument(); | ||
| }); | ||
| }); |
47 changes: 47 additions & 0 deletions
47
src/react/src/__tests__/components/DateTimeProperty.test.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| import React from 'react'; | ||
| import { render, screen } from '@testing-library/react'; | ||
| import moment from 'moment'; | ||
| import DateTimeProperty from '../../components/PartnerFields/FormatComponents/DateTimeProperty'; | ||
|
|
||
| const defaultProps = { | ||
| propertyKey: 'created_at', | ||
| value: { created_at: '2024-06-15T14:30:00Z' }, | ||
| schemaProperties: { created_at: {} }, | ||
| }; | ||
|
|
||
| describe('DateTimeProperty', () => { | ||
| it('renders a formatted date-time from value', () => { | ||
| render(<DateTimeProperty {...defaultProps} />); | ||
|
|
||
| const expected = moment('2024-06-15T14:30:00Z').format('LLL'); | ||
| expect(screen.getByText(expected)).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('falls back to schema default datetime when value is missing', () => { | ||
| render( | ||
| <DateTimeProperty | ||
| {...defaultProps} | ||
| value={{}} | ||
| schemaProperties={{ | ||
| created_at: { default: '2024-01-15T09:00:00Z' }, | ||
| }} | ||
| />, | ||
| ); | ||
|
|
||
| const expected = moment('2024-01-15T09:00:00Z').format('LLL'); | ||
| expect(screen.getByText(expected)).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('displays the title from schema', () => { | ||
| render( | ||
| <DateTimeProperty | ||
| {...defaultProps} | ||
| schemaProperties={{ | ||
| created_at: { title: 'Created At' }, | ||
| }} | ||
| />, | ||
| ); | ||
|
|
||
| expect(screen.getByText(/Created At:/)).toBeInTheDocument(); | ||
| }); | ||
| }); |
37 changes: 37 additions & 0 deletions
37
src/react/src/__tests__/components/DefaultProperty.test.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import React from 'react'; | ||
| import { render, screen } from '@testing-library/react'; | ||
| import DefaultProperty from '../../components/PartnerFields/TypeComponents/DefaultProperty/DefaultProperty'; | ||
|
|
||
| const defaultProps = { | ||
| propertyKey: 'status', | ||
| value: { status: 'active' }, | ||
| schemaProperties: { status: { title: 'Status' } }, | ||
| }; | ||
|
|
||
| describe('DefaultProperty', () => { | ||
| it('renders the string value from value object', () => { | ||
| render(<DefaultProperty {...defaultProps} />); | ||
|
|
||
| expect(screen.getByText('active')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('falls back to schema default when value is missing', () => { | ||
| render( | ||
| <DefaultProperty | ||
| {...defaultProps} | ||
| value={{}} | ||
| schemaProperties={{ | ||
| status: { title: 'Status', default: 'pending' }, | ||
| }} | ||
| />, | ||
| ); | ||
|
|
||
| expect(screen.getByText('pending')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('displays the title from schema', () => { | ||
| render(<DefaultProperty {...defaultProps} />); | ||
|
|
||
| expect(screen.getByText(/Status:/)).toBeInTheDocument(); | ||
| }); | ||
| }); |
48 changes: 48 additions & 0 deletions
48
src/react/src/__tests__/components/PartnerFieldLabel.test.jsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| import React from 'react'; | ||
| import { render, screen } from '@testing-library/react'; | ||
| import { MuiThemeProvider, createMuiTheme } from '@material-ui/core/styles'; | ||
|
|
||
| import PartnerFieldLabel from '../../components/PartnerFields/PartnerFieldLabel/PartnerFieldLabel'; | ||
|
|
||
| const theme = createMuiTheme(); | ||
|
|
||
| const renderPartnerFieldLabel = (props = {}) => | ||
| render( | ||
| <MuiThemeProvider theme={theme}> | ||
| <PartnerFieldLabel title="Test Title" {...props} /> | ||
| </MuiThemeProvider>, | ||
| ); | ||
|
|
||
| describe('PartnerFieldLabel', () => { | ||
| test('renders without crashing', () => { | ||
| const { container } = renderPartnerFieldLabel(); | ||
| expect(container.querySelector('span')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('displays the title followed by a colon', () => { | ||
| renderPartnerFieldLabel({ title: 'Country' }); | ||
| expect(screen.getByText('Country:')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('applies the label class from withStyles', () => { | ||
| const { container } = renderPartnerFieldLabel(); | ||
| const span = container.querySelector('span'); | ||
| expect(span.className).toMatch(/label/); | ||
| }); | ||
|
|
||
| test('renders different titles correctly', () => { | ||
| const { rerender } = render( | ||
| <MuiThemeProvider theme={theme}> | ||
| <PartnerFieldLabel title="Address" /> | ||
| </MuiThemeProvider>, | ||
| ); | ||
| expect(screen.getByText('Address:')).toBeInTheDocument(); | ||
|
|
||
| rerender( | ||
| <MuiThemeProvider theme={theme}> | ||
| <PartnerFieldLabel title="Sector" /> | ||
| </MuiThemeProvider>, | ||
| ); | ||
| expect(screen.getByText('Sector:')).toBeInTheDocument(); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.