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.

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

Jump to bottom

DTRA-2153 / Kate / [DTrader-v2] Closed market fields #17254

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

DTRA-2153 / Kate / [DTrader-v2] Closed market fields #17254

wants to merge 15 commits into from

Conversation

kate-deriv
Copy link
Contributor

@kate-deriv kate-deriv commented Oct 18, 2024 •
edited
Loading

Changes:

  • Added possibility to disable all trade parameters via flag. According to task requirements it will be used in case is market if closed
  • Added dash instead of skeleton loader for current spot for closed market
  • Fixed small issue (when closed market message was shown after page refresh, but the symbol was changed to open market)
  • Refactored trade-parameters.tsx and trade.tsx
  • Refactored tests

Screenshots:

Screenshot 2024-10-21 at 12 00 46 PM
Screenshot 2024-10-21 at 12 00 02 PM
Screenshot 2024-10-21 at 11 09 38 AM
Screenshot 2024-10-21 at 11 09 29 AM
Screenshot 2024-10-21 at 11 09 24 AM
Screenshot 2024-10-21 at 11 09 03 AM
Screenshot 2024-10-21 at 11 08 58 AM

@vercel Vercel
Copy link

vercel bot commented Oct 18, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
deriv-app ✅ Ready (Inspect) Visit Preview Oct 23, 2024 7:29am

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 18, 2024 •
edited
Loading

A production App ID was automatically generated for this PR. (log)

Click here to copy & paste above information. 
- **PR**: [https://github.com/deriv-com/deriv-app/pull/17254](https://github.com/deriv-com/deriv-app/pull/17254)
- **URLs**:
    - **w/ App ID + Server**: https://deriv-app-git-fork-kate-deriv-kate-dtra-2153closedmarketsfields.binary.sx?qa_server=red.derivws.com&app_id=23838
    - **Original**: https://deriv-app-git-fork-kate-deriv-kate-dtra-2153closedmarketsfields.binary.sx
- **App ID**: `23838`

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 18, 2024 •
edited
Loading

🚨 Lighthouse report for the changes in this PR:

Category Score
🔺 Performance 31
🟧 Accessibility 70
🟢 Best practices 92
🟧 SEO 77
🟧 PWA 78

Lighthouse ran with https://deriv-app-git-fork-kate-deriv-kate-dtra-2153closedmarketsfields.binary.sx/

@coveralls
Copy link

coveralls commented Oct 18, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 11474695146

Details

  • 28 of 31 (90.32%) changed or added relevant lines in 21 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 53.733%
Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/trader/src/AppV2/Components/ClosedMarketMessage/closed-market-message.tsx 0 3 0.0%
Totals Coverage Status
Change from base Build 11474094392: 0.02%
Covered Lines: 35167
Relevant Lines: 60893
💛 - Coveralls

@kate-deriv kate-deriv marked this pull request as ready for review October 21, 2024 09:07
kate-deriv
kate-deriv commented Oct 22, 2024
View reviewed changes
packages/trader/src/AppV2/Components/TradeParameters/Duration/duration.tsx
@@ -168,7 +164,7 @@ const Duration = observer(({ is_minimized }: TDurationProps) => {
label={<Localize i18n_default_text='Duration' key={`duration${is_minimized ? '-minimized' : ''}`} />}
value={getInputValues()}
noStatusIcon
disabled={isMarketClosed(activeSymbols, symbol)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ahmadtaimoor-deriv please, help to review this change as you were working with duration component and I might breake something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

activeSymbols from hook is basically something that will be set to the store, while is_market_closed from trade store is based on active_symbols from store directly (as I understand).
Please, @henry-deriv, correct me in case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henry-deriv henry-deriv henry-deriv approved these changes

@nijil-deriv nijil-deriv Awaiting requested review from nijil-deriv nijil-deriv is a code owner

@vinu-deriv vinu-deriv Awaiting requested review from vinu-deriv vinu-deriv is a code owner

@wojciech-deriv wojciech-deriv Awaiting requested review from wojciech-deriv wojciech-deriv is a code owner

@sandeep-deriv sandeep-deriv Awaiting requested review from sandeep-deriv

@shafin-deriv shafin-deriv Awaiting requested review from shafin-deriv

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kate-deriv @coveralls @henry-deriv