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

added a change function color parameter for issue #151 #153

Closed
wants to merge 2 commits into from
Closed

added a change function color parameter for issue #151 #153

wants to merge 2 commits into from

Conversation

AlexandreArduino
Copy link

You can now change the function color as asked in the issue

nw_function_color.mp4

@github-actions
Copy link

github-actions bot commented Feb 8, 2022

.text .rodata .bss .data Total (RAM) Total (ROM)
Base 810776 bytes 442100 bytes 226888 bytes 1776 bytes 228664 bytes 1254652 bytes
Head 811680 bytes 442916 bytes 226888 bytes 1776 bytes 228664 bytes 1256372 bytes
+904 bytes +816 bytes +0 bytes +0 bytes +0 bytes +1720 bytes
+0.1 % +0.2 % +0.0 % +0.0 % +0.0 % +0.1 %

@Yaya-Cout
Copy link
Member

I think something like this (numworks#1385) or this (numworks#1741) will be more readable, and you wrote “vertt” instead of “vert”, but thanks you for this pull request

@AlexandreArduino
Copy link
Author

it is aleady corrected for the "vertt" :p and for numworks#1741 i agree with you, i'll try to modify it later

@RedGl0w
Copy link

RedGl0w commented Feb 8, 2022

it is aleady corrected for the "vertt" :p and for numworks#1741 i agree with you, i'll try to modify it later

Sorry :p

Btw, I think that a more "compact" solution like the one used in numworks#1385 (comment) should be rather used that a list view.

@AlexandreArduino
Copy link
Author

Yes sure but if we want to have the same ui in the whole calculator (numworks#1385 (comment) as said artaxxx) it's better to use a list view but you're right that it's not very compact :p

@Lauryy06
Copy link
Member

Lauryy06 commented Feb 8, 2022

I think the implementation that Redglow showed is indeed better.

@AlexandreArduino
Copy link
Author

okay no problem :p

@RedGl0w
Copy link

RedGl0w commented Feb 8, 2022

I think the implementation that Redglow showed is indeed better.

However, I'm not personally convinced in the square selector (the idea is very good, but it can be a bit not User friendly)
I don't know what would be better, but I still prefer this one to the list menu

Comment on lines 33 to 41
if(color == KDColorBlack) return Message(0);
if(color == KDColorBlue) return Message(1);
if(color == KDColorRed) return Message(2);
if(color == KDColorPurple) return Message(3);
if(color == KDColorGreen) return Message(4);
if(color == KDColorOrange) return Message(5);
if(color == KDColorYellow) return Message(6);
if(color == KDColorWhite) return Message(7);
return Message(2);
Copy link
Member

Choose a reason for hiding this comment

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

You can use a switch here

Copy link
Author

Choose a reason for hiding this comment

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

yes I had this idea but sometimes in the code you use many if(...) instead of a switch so I thought it was what I had to do but 🤔 but i correct it

Copy link
Author

Choose a reason for hiding this comment

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

finally I tried the switch but I've got an error : error: call to non-‘constexpr’ function ‘KDColor::operator uint16_t() const’
case KDColorWhite: return Message(7);
So for now, I remove it :p

@github-actions
Copy link

.text .rodata .bss .data Total (RAM) Total (ROM)
Base 810776 bytes 442100 bytes 226888 bytes 1776 bytes 228664 bytes 1254652 bytes
Head 811680 bytes 442916 bytes 226888 bytes 1776 bytes 228664 bytes 1256372 bytes
+904 bytes +816 bytes +0 bytes +0 bytes +0 bytes +1720 bytes
+0.1 % +0.2 % +0.0 % +0.0 % +0.0 % +0.1 %

@@ -31,3 +31,11 @@ DerivativeFunctionColumn = "Spalte der Ableitungsfunktion"
HideDerivativeColumn = "Ableitungsfunktion ausblenden"
AllowedCharactersAZaz09 = "Erlaubt: A..Z, a..z, 0..9, _"
ReservedName = "Reserviertes Wort"
FunctionColorBlack = "schwarz"
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to start the colors with a capital letter?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know how to do that 🤣 no i'm joking but you think this pr is still valid ? bc it is not very compact 🤔 as said @RedGl0w

Copy link
Member

@Lauryy06 Lauryy06 Feb 15, 2022

Choose a reason for hiding this comment

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

I don't know how to do that

Just change the text...

Suggested change
FunctionColorBlack = "schwarz"
FunctionColorBlack = "Schwarz"

(and add a line at the end of each file please)

Copy link

Choose a reason for hiding this comment

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

For the name of the colors, instead of somehow duplicating them, couldn't you rather use the pythonColors ?

Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to move the Python colour names in the shared i18n file in this case to can build without the Python app.

Copy link

@RedGl0w RedGl0w left a comment

Choose a reason for hiding this comment

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

Some epsilon coding style fixes

FunctionColorGreen = "grün"
FunctionColorOrange = "Orange"
FunctionColorYellow = "gelb"
FunctionColorWhite = "weiß"
Copy link

Choose a reason for hiding this comment

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

Add carriage return (for all files which don't have it)

Copy link
Author

Choose a reason for hiding this comment

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

okay i do that

#include "color_list.h"
#include "apps/i18n.h"

namespace Graph {
Copy link

Choose a reason for hiding this comment

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

Don't indent after namespace (for all files)


namespace Graph {
I18n::Message Message(int index) {
static constexpr I18n::Message message[NumberOfColors] = {
Copy link

Choose a reason for hiding this comment

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

Indentation should be made of 2 spaces (for all files)

@@ -0,0 +1,12 @@
#pragma once
Copy link

Choose a reason for hiding this comment

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

Suggested change
#pragma once
#ifndef GRAPH_COLOR_LIST_H
#define GRAPH_COLOR_LIST_H

This should be done in all header

@Yaya-Cout
Copy link
Member

Can you add a space before the function colour arrow ? (it is ugly without space)
arrow without space
Can you also show the colour in the submenu, like numworks#1741 ? (the white colour isn't visible when it isn't selected)

@Yaya-Cout Yaya-Cout linked an issue Mar 2, 2022 that may be closed by this pull request
This pull request was closed.
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.

Redefinir les couleurs des courbes
4 participants