Andras/BoundlessFlowCampus2K
4
7
Fork 0
Code Issues 90 Pull requests 7 Projects Releases Packages Wiki Activity Actions

#989 Create a function that creates graphs. #1150

Closed
a21jonfa wants to merge 7 commits from #989 into both_teams_week_8
pull from: #989
merge into: Andras:both_teams_week_8
Conversation 3 Commits 7 Files changed 4 +204
a21jonfa commented 2026-05-26 01:00:33 +00:00
Collaborator
Copy link

Added a Graph component with line graph so far fully supported.
features:

  • resizable
  • dynamic scale of the y-axis
  • can display up to 5 properties.
    i think alot of features can be stripped out and used for other types of graphs in the future making them much easier to implement.
    #989
Added a Graph component with line graph so far fully supported. features: * resizable * dynamic scale of the y-axis * can display up to 5 properties. i think alot of features can be stripped out and used for other types of graphs in the future making them much easier to implement. #989
Capture.PNG
24 KiB
Capture2.PNG
18 KiB
Capture.PNG Capture2.PNG
c24danli 2026-05-26 06:28:57 +00:00
b25marem commented 2026-05-26 06:59:34 +00:00
Collaborator
Copy link

Review on #989

Whats done

A reusable Graph component has been added. The current implementation supports line graphs with dynamic resizing, y-axis labels, graph dots, and a legend.

Tested on Windows using Chrome

  • App starts.
  • Inspected the implementation and reviewed the screenshots provided in the PR.
  • The graph could not be fully tested directly in the application because the test view is not connected to the dashboard/routing.
  • This is also stated in the code comment in TestGraph.js.

Inspected following files

  • frontend/src/components/Graphs/Graph.js

    • Indentation/code styling

      • Mostly readable and consistent.

    • Naming convention

      • Component names follow PascalCase:

        • Graph
        • LineGraph
        • BarGraph
        • PieGraph

      • Variable names mostly follow camelCase.

    • Printed text uses and follows localization standards

      • Hardcoded visible text exists and should use localization if shown to users:

        • No valid data availible
        • "Is not a valid/supported graph type."
        • bar
        • pie

      • Spelling issue: availible should be available.

    • Comments

      • Comments are understandable, but some should be shorter and more formal.

    • General code quality

      • The line graph implementation looks reusable.

      • Missing React key props in some mapped SVG elements.

      • BarGraph and PieGraph are currently only placeholders.

      • Edge cases should be handled better, for example:

        • only one data point
        • equal min and max y-values
        • more than 5 properties

      • Inline styles and hardcoded colors are used.

  • frontend/src/features/Graphs/TestGraph.js

    • Indentation/code styling

      • Mostly readable.

    • Naming convention

      • TestGraphView follows PascalCase.
      • Data should be renamed to data according to camelCase variable naming convention.

    • Printed text uses and follows localization standards

      • No localization issue found in this file.

    • Comments

      • The comment clearly explains that the view is not connected to the dashboard.

    • General code quality

      • Useful as a mock/test view.
      • The view is not connected to the dashboard/routing, so the graph cannot be tested directly through the app.

What needs to be done

  • frontend/src/components/Graphs/Graph.js

    • Add missing React key props for mapped elements.
    • Fix spelling: availible → available.
    • Move visible hardcoded text to localization if it can be shown to users.
    • Handle edge cases for one data point, equal y-values, and too many properties.
    • Consider moving inline styles and hardcoded colors to CSS/classes or constants.
    • Consider removing or hiding unsupported bar and pie options until they are implemented.

  • frontend/src/features/Graphs/TestGraph.js

    • Rename Data to data.
    • Connect the test graph view to routing/dashboard or provide clear testing instructions for reviewers.

Conclusion

  • The line graph implementation looks promising and the screenshots show the intended result.
  • However, the graph could not be fully tested directly in the application because the test view is not connected to the dashboard/routing.
  • The PR should be improved before merge, mainly by making the graph testable in the app and fixing the React/key, naming, localization, and edge case issues.
# Review on #989 ## Whats done A reusable `Graph` component has been added. The current implementation supports line graphs with dynamic resizing, y-axis labels, graph dots, and a legend. ### Tested on Windows using Chrome * App starts. * Inspected the implementation and reviewed the screenshots provided in the PR. * The graph could not be fully tested directly in the application because the test view is not connected to the dashboard/routing. * This is also stated in the code comment in `TestGraph.js`. ### Inspected following files * `frontend/src/components/Graphs/Graph.js` * Indentation/code styling * Mostly readable and consistent. * Naming convention * Component names follow PascalCase: * `Graph` * `LineGraph` * `BarGraph` * `PieGraph` * Variable names mostly follow camelCase. * Printed text uses and follows localization standards * Hardcoded visible text exists and should use localization if shown to users: * `No valid data availible` * `"Is not a valid/supported graph type."` * `bar` * `pie` * Spelling issue: `availible` should be `available`. * Comments * Comments are understandable, but some should be shorter and more formal. * General code quality * The line graph implementation looks reusable. * Missing React `key` props in some mapped SVG elements. * `BarGraph` and `PieGraph` are currently only placeholders. * Edge cases should be handled better, for example: * only one data point * equal min and max y-values * more than 5 properties * Inline styles and hardcoded colors are used. * `frontend/src/features/Graphs/TestGraph.js` * Indentation/code styling * Mostly readable. * Naming convention * `TestGraphView` follows PascalCase. * `Data` should be renamed to `data` according to camelCase variable naming convention. * Printed text uses and follows localization standards * No localization issue found in this file. * Comments * The comment clearly explains that the view is not connected to the dashboard. * General code quality * Useful as a mock/test view. * The view is not connected to the dashboard/routing, so the graph cannot be tested directly through the app. ## What needs to be done * `frontend/src/components/Graphs/Graph.js` * Add missing React `key` props for mapped elements. * Fix spelling: `availible` → `available`. * Move visible hardcoded text to localization if it can be shown to users. * Handle edge cases for one data point, equal y-values, and too many properties. * Consider moving inline styles and hardcoded colors to CSS/classes or constants. * Consider removing or hiding unsupported `bar` and `pie` options until they are implemented. * `frontend/src/features/Graphs/TestGraph.js` * Rename `Data` to `data`. * Connect the test graph view to routing/dashboard or provide clear testing instructions for reviewers. ## Conclusion * The line graph implementation looks promising and the screenshots show the intended result. * However, the graph could not be fully tested directly in the application because the test view is not connected to the dashboard/routing. * The PR should be improved before merge, mainly by making the graph testable in the app and fixing the React/key, naming, localization, and edge case issues.
a22erigr changed target branch from team_2_week_7 to both_teams_week_8 2026-05-27 08:57:18 +00:00 
Added missing React keys, localized text, fixed spelling, removed unused bar and pie placeholders.
a24hirsa commented 2026-05-28 10:27:29 +00:00
Collaborator
Copy link

Review on #989, PR #1150

Whats done

Reviewed the new graph code changes and testing app from latest commit makign changes based on previous review

Tested on Linux using Firefox

  • App starts after running an Installation
  • graph could not be tested as expected and described by previous review

Inspected following files

  • frontend/src/components/Graphs/Graph.js
    • Indentation/code styling
      • Follows our prettier code standard and is correctly indented
    • General Code quality
      • code quality is good and translations are now used correctly
      • unnecessary elements have been removed
  • frontend/src/features/Graphs/TestGraph.js
    • Code Styling
      • variable names have now be turned into camelCase and follow our standards
  • frontend/src/locales/en.json and sv.json
    • Spelling
      • Spelling is correct for both english and swedish versions

What needs to be done

  • create future issues to make a usable card or graph utilizing this system to be tested

Conclusion

  • Everything looks good and should be ready to merge
  • create issues related to creating testable and usable cards of this system
# Review on #989, PR #1150 ## Whats done Reviewed the new graph code changes and testing app from latest commit makign changes based on previous review ### Tested on Linux using Firefox + App starts after running an Installation + graph could not be tested as expected and described by previous review ### Inspected following files + frontend/src/components/Graphs/Graph.js + Indentation/code styling + Follows our prettier code standard and is correctly indented + General Code quality + code quality is good and translations are now used correctly + unnecessary elements have been removed + frontend/src/features/Graphs/TestGraph.js + Code Styling + variable names have now be turned into camelCase and follow our standards + frontend/src/locales/en.json and sv.json + Spelling + Spelling is correct for both english and swedish versions ## What needs to be done + create future issues to make a usable card or graph utilizing this system to be tested ## Conclusion + Everything looks good and should be ready to merge + create issues related to creating testable and usable cards of this system
a22erigr commented 2026-05-28 10:43:18 +00:00
Collaborator
Copy link

because we can't see the graph creation in a view this will not be merged.

because we can't see the graph creation in a view this will not be merged.
a22erigr closed this pull request 2026-05-28 10:43:19 +00:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No reviewers
b25marem
a24hirsa
Labels
Clear labels   
_CRITICAL_

Only use this label for critical tasks that are project-scope,and must be addressed ASAP.

  
API

   
app

   
backEnd

   
Blocked, waiting for further changes

   
bug

Something is not working

  
cleanup

Issues to cleanup the codebase (restructure folders, remove deprecated or smelly code)

  
close

Issue to close

  
design

   
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
feature request

   
frontEnd

   
help wanted

Need some help

  
invalid

Something is wrong

  
low priority

   
needs input

Issue needs input from product owner or customer

  
needs review

issue is in need of review

  
project documentation

   
question

More information is needed

  
research

   
reviewed

This Issue has been reviewed

  
script

   
security

use this for security-related issues

  
SQL

  
style

   
testing

   
topLevel

Reserved for top-level, project-wide issues.

  
wontfix

This won't be fixed

No labels
_CRITICAL_
API
app
backEnd
Blocked, waiting for further changes
bug
cleanup
close
design
duplicate
enhancement
feature request
frontEnd
help wanted
invalid
low priority
needs input
needs review
project documentation
question
research
reviewed
script
security
SQL
style
testing
topLevel
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
c24carso
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
Andras/BoundlessFlowCampus2K!1150
No description provided.