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

Feature/1342 rejseplanen rewrite to fit new api WIP #243

Merged

Conversation

sinejespersen
Copy link
Contributor

@sinejespersen sinejespersen commented May 16, 2024

Link to ticket

https://leantime.itkdev.dk/dashboard/home#/tickets/showTicket/1342

Description

  • Add entry in example config for midttrafik api key
  • Clean up multi select component a bit, replace reduce with Map logic, as reduce that made it unnecessarily complicated to read.
  • Make the station selector call new api
  • Add config to context in app.jsx
  • Make the loading screen surpass the menu
  • Skip some tests and add todos

Screenshot of the result

image

Checklist

  • My code is covered by test cases.
  • My code passes our test (all our tests).
  • My code passes our static analysis suite.
  • My code passes our continuous integration process.

If your code does not pass all the requirements on the checklist you have to add a comment explaining why this change
should be exempt from the list.

Additional comments or questions

os2display/display-templates#153

@@ -55,7 +55,7 @@ body,
display: flex;
justify-content: center;
padding: 5em;
z-index: 10;
z-index: 1021;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid this:

image

@sinejespersen sinejespersen requested a review from tuj May 27, 2024 06:32
@sinejespersen sinejespersen marked this pull request as ready for review May 27, 2024 06:33
@@ -1,6 +1,7 @@
{
"api": "/",
"touchButtonRegions": false,
"rejseplanenApiKey": "abc",
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be added to the infrastructure templates.

Copy link
Contributor

Choose a reason for hiding this comment

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

The default value should be null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is "the infrastructure templates"?

Copy link
Contributor

Choose a reason for hiding this comment

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

src/components/slide/content/station/station-selector.jsx Outdated Show resolved Hide resolved
src/components/slide/content/station/station-selector.jsx Outdated Show resolved Hide resolved
src/components/playlist/campaign.spec.js Show resolved Hide resolved
@@ -23,7 +23,7 @@ describe("Playlist pages work", () => {
cy.get("#save_playlist").should("exist");
});

it("It drags and drops slide", () => {
it.skip("It drags and drops slide", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why skip?
Add comment

/**
* A callback on changed data.
*
* @param {Array} data The data to call back with
*/
const changeData = (data) => {
let selectedOptions = [];

Copy link
Contributor

Choose a reason for hiding this comment

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

New lines around if statements increase the readability of the code.

* @returns {Array} An array of {label, value, disabled}
*/
function mapDataToFitMultiselect(dataToMap) {
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

why ( ) around the return value?
It is only needed for jsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the linter does it

@sinejespersen sinejespersen requested a review from tuj May 29, 2024 07:39
@@ -1,6 +1,7 @@
{
"api": "/",
"touchButtonRegions": false,
"rejseplanenApiKey": "abc",
Copy link
Contributor

Choose a reason for hiding this comment

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

@tuj tuj added the enhancement New feature or request label May 29, 2024
@sinejespersen sinejespersen requested a review from tuj May 29, 2024 13:06
@sinejespersen sinejespersen requested a review from tuj May 31, 2024 08:13
@sinejespersen sinejespersen merged commit f64b131 into develop Jun 3, 2024
4 checks passed
@sinejespersen sinejespersen deleted the feature/1342-rejseplanen-rewrite-to-fit-new-api branch June 3, 2024 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants