charts enhancement #113
charts enhancement #113
Conversation
updated source files to upstream main
…" to use current candle
- resized the chart - changed color for chart timescale and pricescale to be more visible
…/dexter into redux-charts-enhancement
✅ Deploy Preview for chipper-sunburst-578cfe ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
src/app/redux/priceChartSlice.ts
Outdated
| //Aysnc Thunk for the update the legend using current candle when site is first loaded | ||
| export const fetchCandlesForInitialPeriod = createAsyncThunk( | ||
| "priceChart/fetchCandlesForInitialPeriod", | ||
| async (_, { dispatch }) => { | ||
| const candlesMap = adex.clientState.currentPairCandlesList; | ||
| const ohlcvData = convertAlphaDEXData(candlesMap); | ||
| if (ohlcvData && ohlcvData.length > 0) { | ||
| const latestOHLCVData = ohlcvData[ohlcvData.length - 1]; | ||
|
|
||
| dispatch(setLegendCandlePrice(latestOHLCVData)); | ||
| dispatch(setLegendChange(latestOHLCVData)); | ||
| dispatch( | ||
| setLegendPercChange({ | ||
| currentOpen: latestOHLCVData.open, | ||
| currentClose: latestOHLCVData.close, | ||
| }) | ||
| ); | ||
| dispatch(setLegendCurrentVolume(latestOHLCVData.value)); | ||
| } | ||
| } | ||
| ); | ||
|
|
There was a problem hiding this comment.
I don't think we need to create a separate thunk for that, we are not doing any network requests here or something like this, all the data comes from adex - we need to update every time adex updates (and this is just not the same as triggering it in useEffect (it will most likely work because data variable will be updated more often than anything else, but still it's not the most straightforward way).
I suggest to create updateAdex in priceChartSlice reducers, accept action: PayloadAction<adex.StaticState> as the second param there, and update all the necessary state changes from it, this way we won't miss the updates and we won't need to call fetch... in the component (we already subscribe to the changes of adex state).
There was a problem hiding this comment.
i refactored the code not to use a thunk. Important to note this is only meant to initialize the legend only when the page is first loaded or move to a different time selector.
Since the crosshair is functional and updates the legend to the candle pointed, I didn't want the legend to update to the current candle data when a new candle is printed. It might be disruptive if a user is on a 5m timeframe and points to a certain candle.
There was a problem hiding this comment.
I don't really get the reason behind initializeLegend, why can't we always show what adex provides? Like you did in priceInfoSlice.
There was a problem hiding this comment.
@EvgeniiaVak I have tried the approach initialize the data with adex.clientState.currentPairCandlesList (similar to what you mentioned priceInfoSlice). The issue might be with initialization timing, the state is being initialized before the data in adex.clientState.currentPairCandlesList is populated thus making the legends all initially empty when i first load the page.
I purposely wanted to prevent updating the legend when a new candle appears that why i did not go with the approach to update the legend on state change.
Maybe making this padding smaller? |
that space doesn't have contain any padding. That is coming from lightweight charts. I am still looking into if there is an option to remove that from the chart for a future PR since I wanted avoid doing something like negative margins. |
|
|
|
oh, I there probably was a reason for this padding... now on some screen widths the chart jumps on the page (becomes taller0 when there are lots of digits displayed: but it definitely looks better without the padding when there are few digits 🤔 maybe we can round up the digits? we have functions for that in utils, but I don't know if this is a place where traders expect precision |
|
It should be fine to round these numbers. No one should be interested in more than 8 decimals as that is what AlphaDEX works with. |
in the chart we can choose not only 1 day period, it can be something different like 5 minutes, so it's not always duplicating info and I think it's ok to always have the price info also |
| volumeSeries.setData( | ||
| data.map((datum) => ({ | ||
| ...datum, | ||
| color: datum.close - datum.open <= 0 ? "#ef5350" : "#26a69a", |
There was a problem hiding this comment.
These colors should come from css variables. As strings in code they can be represented as hsl(var(--su)) for green, hsl(var(--er)) for red.
There was a problem hiding this comment.
When I tried to use the hsl(var(--su)) variables it cause the chart to be black (randomly hit or miss), I suspect it was because the library does some extra steps with the string
There was a problem hiding this comment.
Ok, probably we can import tailwind config object then and take the color from there (I guess only if it goes inside the canvas), we'll need to handle dark/light switching ourselves (but that's after-mvp).
There was a problem hiding this comment.
I did find this fiddle that will help with that after the mvp.
https://jsfiddle.net/TradingView/6yLzrbtd/
| background: { color: "#181c27" }, | ||
| textColor: "#DDD", | ||
| }, | ||
| //MODIFY THEME COLOR HERE | ||
| grid: { | ||
| vertLines: { color: "#444" }, | ||
| horzLines: { color: "#444" }, | ||
| }, | ||
| timeScale: { | ||
| //MODIFY THEME COLOR HERE | ||
| borderColor: "#d3d3d4", |
There was a problem hiding this comment.
These colors probably can also come from the theme settings. You can find what colors we currently have at tailwind.config.js under daisyui objects (only dark theme currently).
Here is the list of color names and variables that you can use - https://daisyui.com/docs/colors/#-2 . Here in the chart settings you can use them as variables (e.g. hsl(var(--b1))). In other places where you style an html element in .tsx you can specify them as classes, e.g. for background bg-base-100 or for text color text-base-100.
| className={`px-[0.5vw] py-[0.5vw] text-sm font-roboto text-#d4e7df hover:bg-white hover:bg-opacity-30 hover:rounded-md ${ | ||
| candlePeriod === period ? "text-blue-500" : "" | ||
| }`} | ||
| onClick={() => dispatch(setCandlePeriod(period))} | ||
| > | ||
| {period} | ||
| </button> | ||
| ))} | ||
| </div> | ||
| <div className="flex justify-between text-sm font-roboto"> | ||
| <div className="ml-4"> | ||
| Open:{" "} | ||
| <span | ||
| className={isNegativeOrZero ? "text-red-500" : "text-green-500"} | ||
| > | ||
| {candlePrice?.open} | ||
| </span> | ||
| </div> | ||
| <div> | ||
| High:{" "} | ||
| <span | ||
| className={isNegativeOrZero ? "text-red-500" : "text-green-500"} |
There was a problem hiding this comment.
Here and below (and you probably can specify {isNegativeOrZero ? ...} one time in the enclosing div (the one with the <div className="flex justify-between) also please use the theme colors to make page look consistent.
Please also remove font-roboto I think it should be set up for the whole app.
There was a problem hiding this comment.
I removed the font-roboto and swapped colors where I could in #145
|
I think I updated the code when you saw this that why it no longer showed that large spacing between the legend and chart.
|
That's fine, given that we can shorten them we probably won't have this problem. @maelsar would you like to update this PR with using utils for shorter numbers and other changes I requested above (styling using the theme config, etc.)? |
|
these commits are included in #145 |
fixes #97
fixes #74
NOTE: I still have further improvements in the pipeline (add more chart options, formatting, layout), but this should be stable enough for the MVP and for testing by the community in the meantime.
#74
In the meantime, if its alright, I will work on the Price Info component so we can get most of the components ready for the MVP.
