Repository navigation
fixing issue 162 + issue 233 - #270
benaliabderrahmane wants to merge 6 commits into
Conversation
… duration with a specific format
| % - For other OSs uses java to retrieve the user.home directory | ||
|
|
||
| if ispc | ||
| % userDir = winqueryreg('HKEY_CURRENT_USER',... |
There was a problem hiding this comment.
Do we need to keep these comments? I don't understand if they are helpful?
There was a problem hiding this comment.
which one ?
| % case 2 | ||
| % param = oauth.percentEncodeString(params{i}); | ||
| % value = oauth.percentEncodeString(params{i+1}); | ||
| % header = http_getContentTypeHeader(1); |
| end | ||
| function [str,header] = http_paramsToString(params,encodeOption) |
There was a problem hiding this comment.
why are these entire files being recommitted? It doesn't look like they have changed at all? This makes it very difficult to review your code.
| if isnumeric(axis_data.XLim) | ||
| xaxis.range = axis_data.XLim; | ||
|
|
||
| elseif isduration(axis_data.XLim) |
| if strcmp(axis_data.YTickLabelMode,'auto') | ||
|
|
||
| %-xaxis range-% | ||
| if isnumeric(axis_data.YLim) |
There was a problem hiding this comment.
Is there a way that you can make this more DRY? It seems like you have essentially the same code for the X and Y access. Can you put this into a function that works for both axes for example? https://en.wikipedia.org/wiki/Don%27t_repeat_yourself
There was a problem hiding this comment.
actually a lot of field are changed and we're not changing the same field each time so it won't be optimal to write everything in a function don't you think
| @@ -0,0 +1,26 @@ | |||
| function [converted,type] = convertDuration(duration) | |||
| if (strcmpi(duration.Format,'s')) | |||
There was a problem hiding this comment.
should be using a switch/case statement?
delete spacing changes
|
It's supporting DataTime and Duration objects. Next step is to make X-axis ticks match!. I guess you could ask @jackparmer to create a separate issue so it counts as multiple fixed issues for you. |
|
Yeah, the x-axis range looks different in the 2 figures. Please solve that within this pull request. |
|
@benaliabderrahmane what is the status of this? Are you ready for another review? |
benaliabderrahmane
left a comment
benaliabderrahmane
left a comment
There was a problem hiding this comment.
no I changed the statut to draft, actually I'm re-writing all the updateAxes code to make it more DRY, and this pull request is going to close 3 issue at a time. I'm going to update the code tomorrow
| if strcmp(axis_data.YTickLabelMode,'auto') | ||
|
|
||
| %-xaxis range-% | ||
| if isnumeric(axis_data.YLim) |
There was a problem hiding this comment.
actually a lot of field are changed and we're not changing the same field each time so it won't be optimal to write everything in a function don't you think
|
plotly now support when one or both data vectors are Datetime or Duration objects.