Skip to content

feat: change version view modifiedOnly default to true #11794

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 27, 2025
Merged

feat: change version view modifiedOnly default to true #11794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
69314c8
feat: change version view `modifiedOnly` default to `true`
philipp-tailor Mar 27, 2025
385093d
test: adopt version e2e test to changed default
philipp-tailor Mar 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/next/src/views/Version/Default/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ export const DefaultVersionView: React.FC<DefaultVersionsViewProps> = ({
current.set('localeCodes', JSON.stringify(selectedLocales.map((locale) => locale.value)))
}

if (!modifiedOnly) {
current.delete('modifiedOnly')
if (modifiedOnly === false) {
current.set('modifiedOnly', 'false')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@GermanJablo Doesn't this change mean that every time the versions list is opened, there will be a client side routing adding the ?modifiedOnly=false query parameter?

IMHO query params should only indicate differences from the default state. Pushing URL changes trigger downstream effects like unnecessary route related computing and possibly re-rendering. That's why I didn't go for changing the URL query params in my initial commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, because modifiedOnly is now true by default. When you open the version list, it will be true, and therefore this condition won't be executed. To be sure, I've verified this with a console.log.

IMHO, query params should only indicate differences from the default state.

That's exactly what I'm doing now. Again, keep in mind that modifiedOnly is now true by default, so we only want to have false in the URL.

The conditions here are only for changing the state when the checkbox is checked or unchecked.

Your initial commit also had a bug where if you manually wrote modifiedOnly in the URL (I don't remember if it was with true, false, or both), the state wasn't reflected correctly.

} else {
current.set('modifiedOnly', 'true')
current.delete('modifiedOnly')
}

const search = current.toString()
Expand Down
2 changes: 1 addition & 1 deletion packages/next/src/views/Version/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export async function VersionView(props: DocumentViewServerProps) {
: null
const comparisonVersionIDFromParams: string = searchParams.compareValue as string

const modifiedOnly: boolean = searchParams.modifiedOnly === 'true'
const modifiedOnly: boolean = searchParams.modifiedOnly === 'false' ? false : true

const { localization } = config

Expand Down
4 changes: 2 additions & 2 deletions test/versions/e2e.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1323,11 +1323,11 @@ describe('Versions', () => {

const textInArrayES = page.locator('[data-field-path="arrayLocalized"][data-locale="es"]')

await expect(textInArrayES).toContainText('No Array Localizeds found')
await expect(textInArrayES).toBeHidden()

await page.locator('#modifiedOnly').click()

await expect(textInArrayES).toBeHidden()
await expect(textInArrayES).toContainText('No Array Localizeds found')
})

test('correctly renders diff for block fields', async () => {
Expand Down