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

Impelment UISK Tokens into FX/Dark Mode #2333

Open
wants to merge 27 commits into
base: SU-15/UISK
Choose a base branch
from
Open

Conversation

SHolleworth
Copy link
Contributor

No description provided.

@SHolleworth SHolleworth changed the title Impelment UISK Token into FX Impelment UISK Tokens into FX Nov 28, 2024
@SHolleworth SHolleworth changed the base branch from master to SU-15/UISK November 28, 2024 13:34
@SHolleworth SHolleworth changed the title Impelment UISK Tokens into FX Impelment UISK Tokens into FX/Dark Mode Nov 28, 2024
Copy link

github-actions bot commented Dec 3, 2024

(auto-deploy) A deployment has been created for this Pull Request

Preview links

As part of the code review process, please ensure that you test against the following

Version URL
Web https://web.env.reactivetrader.com/pull/2333
OpenFin - FX fins://openfin.env.reactivetrader.com/pull/2333/config/rt-fx.json
OpenFin - Credit fins://openfin.env.reactivetrader.com/pull/2333/config/rt-credit.json
OpenFin - Launcher fins://openfin.env.reactivetrader.com/pull/2333/config/launcher.json
OpenFin - Workspace fins://openfin.env.reactivetrader.com/pull/2333/workspace/config/workspace.json
Finsemble https://finsemble.env.reactivetrader.com/pull/2333

Performance

Please ensure that this PR does not degrade the performance of the UI. We should maintain a performance score of 95+.

https://developers.google.com/speed/pagespeed/insights/?url=https://web.env.reactivetrader.com/pull/2333

Copy link
Contributor

@algreasley algreasley left a comment

Choose a reason for hiding this comment

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

quick pass .. more later

packages/client/package.json Outdated Show resolved Hide resolved
const [hovering, setHovering] = useState(false)
const color = basePnl >= 0 ? "positive" : "negative"
const color = profitOrLossValue >= 0 ? "positive" : "negative"
Copy link
Contributor

Choose a reason for hiding this comment

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

can we map colors in the theme, so we reference the pos/neg colors by name - I am doing this in RTEq MUI theme, as we did in UISK v1

Copy link
Contributor

Choose a reason for hiding this comment

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

will clear your E2E failure too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed in the tweaks pr, the pnl bars dont change colour anymore

basePnl: -849.4217864553628,
maxVal: 1785.9428620191757,
profitOrLossValue: -849.4217864553628,
largetProfitOrLossValue: 1785.9428620191757,
Copy link
Contributor

Choose a reason for hiding this comment

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

typo - restore the type info, perhaps - it needs to be corrected there, too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in the tweaks pr

const AnalyticsRegion = styled(Region)`
width: 320px;
background-color: ${({ theme }) =>
theme.newTheme.color["Colors/Background/bg-primary"]};
Copy link
Contributor

Choose a reason for hiding this comment

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

thinking about this .. looks awkward .. no immediate actions, but I'd like to do better around color refs .. in RTEq I am doing all the fiddly mapping in theme.ts

@@ -43,13 +42,13 @@ export const Analytics = ({ hideIfMatches = "(max-width: 750px)" }: Props) => {

return (
<AnalyticsWrapper ref={ref} hideIfMatches={hideIfMatches}>
<Suspense fallback={loader}>
<RegionWrapper fallback={loader}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've refactored all the major lazy load wrappers (live rates, rfqs, etc) to use this RegionWrapper, as they were all doing basically the same thing.

{supportsTearOut && <TearOutComponent section="analytics" />}
</RightNav>
</AnalyticsHeader>
<AnalyticsRegion
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've refactored all the sections of the app to use this Region component, since their layouts and stuctures all share a lot of commanalities.

source$={analytics$}
fallback={children}
Header={
<TabBar
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The headers for all the regions are now using the tabbar component, which also renders all the icon buttons (tear out, tile chart view, etc) called actions here


import { SectionLayout } from "@/client/components/layout/SectionLayout"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I should rename this to AnalyticsSectionLayout?

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.

2 participants