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

use round() in transforms #2064

Merged
merged 7 commits into from
May 24, 2024
Merged

use round() in transforms #2064

merged 7 commits into from
May 24, 2024

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented May 23, 2024

Changes

This PR helps with #1551 by rounding some percentage values to the nearest pixel, using the brand-new round() CSS function. This is mainly in transforms in dialog and expandable-block. And for browsers that don't support round(), the unrounded fallback will continue to be used.

Unrelated to the round() change but might possibly help towards #1551: simplified information-panel to remove transform: translate(0).

I don't think this PR is the "ultimate" fix, so I'm not linking the issue to be closed. But it's a small net improvement nonetheless.

Implementation notes:

  • I had to use Round() instead of round() to disambiguate from the Sass round() function.
  • I had to add @supports blocks which would normally be unnecessary, because lightningcss was trying to "minify" the CSS by removing repeated instances of the same property.

Testing

I tried to test in my Windows VM with some of the sandboxes linked in #1551. It was somewhat difficult to reproduce the issues in the first place, but I did see an improvement in at least one situation:

Screenshots
Before After
blurry text in expandable-block inside dialog sharp text in same expandable-block inside same dialog

Further testing on a real Windows machine would be useful. (Make sure to update to Chrome 125)

Docs

Added changesets.

@mayank99 mayank99 self-assigned this May 23, 2024
@mayank99 mayank99 marked this pull request as ready for review May 23, 2024 21:49
@mayank99 mayank99 requested review from a team as code owners May 23, 2024 21:49
@mayank99 mayank99 requested review from r100-stack and Ben-Pusey-Bentley and removed request for a team May 23, 2024 21:49
Ben-Pusey-Bentley

This comment was marked as outdated.

@mayank99

This comment was marked as outdated.

@Ben-Pusey-Bentley

This comment was marked as outdated.

@mayank99

This comment was marked as resolved.

@Ben-Pusey-Bentley
Copy link
Contributor

Ben-Pusey-Bentley commented May 24, 2024

@Ben-Pusey-Bentley I don't see much of a difference (the "Before" text is not even blurry, at least in the screenshots). It would be nice to find a reproducible issue first. Try this and this and this

This code shows the improvement better:

Code
import * as React from 'react';
import {
  ExpandableBlock,
  Flex,
  Modal,
  ModalContent,
  Table,
  ToggleSwitch,
} from '@itwin/itwinui-react';

export default function App({
  isDvh,
  setIsDvh,
}: {
  isDvh: boolean;
  setIsDvh: (value: boolean) => void;
}) {
  const packageInfo = {
    '@bentley/documents-management': '2.1.7-dev.0',
    '@bentley/formsrenderer': '1.102.0',
    '@bentley/office365-collaboration-login-modal': '1.2.0',
    '@bentley/office365-collaboration-settings': '1.2.2',
    '@bentley/pw-api': '1.1.0-dev.1',
    '@bentley/pw-i18n': '1.0.4-dev.0',
    '@bentley/pwnxt-document-picker': '3.4.1',
    '@bentley/supply-chain-components': '0.0.3-beta.1',
    '@itwin/itwinui-react': '2.12.19',
  };

  return (
    <>
      <Modal isOpen={true} title="Connection info">
        <ModalContent>
          <Flex flexDirection="column" alignItems="stretch">
            <ToggleSwitch
              label="Use DVH units"
              defaultChecked={isDvh}
              onChange={() => setIsDvh(!isDvh)}
            />
            <ExpandableBlock
              title="Package versions"
              size="small"
              isExpanded={true}
            >
              <Table
                columns={[
                  { Header: 'Package', accessor: 'name' },
                  { Header: 'Version', accessor: 'version' },
                ]}
                emptyTableContent="No data"
                data={Object.entries(packageInfo).map(([name, version]) => ({
                  name,
                  version,
                }))}
                density="extra-condensed"
                style={{ maxHeight: '200px' }}
              />
            </ExpandableBlock>
          </Flex>
        </ModalContent>
      </Modal>
    </>
  );
}

Before:
image

After:
image

The header text is still blurry in the after image, but the text inside of the table rows isn't blurry anymore.

@mayank99 mayank99 merged commit 75a7a5f into main May 24, 2024
16 checks passed
@mayank99 mayank99 deleted the mayank/rounded-transforms branch May 24, 2024 22:28
@imodeljs-admin imodeljs-admin mentioned this pull request May 24, 2024
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.

3 participants