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

Update launcher to use the Design System #82

Merged
merged 41 commits into from
Jul 8, 2024
Merged

Conversation

VirajP1002
Copy link
Contributor

@VirajP1002 VirajP1002 commented Jun 7, 2024

What is the context of this PR?

To update the front end of Launcher, we decided to use DS components. However, given the fact that Launcher in written in Go, using the macros wasn't possible so the HTML had to be hardcoded within the templates files and the components were imported using the CDN link.

The changes to launch.html include creating to separate columns in order for the right side of the page to be utilised. The first column includes the options for selecting a schema either by name or using CIR. The second column is the metadata section, it uses the accordion component which dynamically updates the metadata sections so only the relevant metadata appears. For example, if you choose a business schema it shows the business metadata, survey metadata, required metadata and runner metadata sections and if you select a test schema it removes the business metadata section and updates the remaining sections. The accordion also has a save state so if you opened a specific section, opened a survey and went back to launcher (only using the back arrow), it would remain open.

The changes to layout.html includes a CDN link for the main.css file. The changes to static/css/main.css includes an update of the background colour of Launcher and removing any redundant CSS classes as the DS handles this. The changes to launch.js includes updates to support the accordion and to remove any redundant JS.

(Note: The accordion elements are hidden rather than injected in via JS because the DS adds attributes to the accordion elements on page load and the JS would be injected after page load hence the attributes needed won't be present).

How to review

  • Make sure to clear your browser cache before reviewing
  • Check the changes made to all the CSS/JS/HTML files
  • Check the formatting of the files
  • Ensure the functionality of Launcher is still the same and everything works as intended
  • Check if any redundant CSS/JS/HTML has been missed.

@VirajP1002 VirajP1002 marked this pull request as ready for review June 7, 2024 09:49
Copy link
Contributor

@rmccar rmccar left a comment

Choose a reason for hiding this comment

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

Noticed these small issues but also theres a tabbing issue where because of the way you are adding details components when you tab through the inputs you are able to focus on the details components which is confusing because they aren't there yet. This is because there is JS from the DS adding tabindex=0 to those elements when they are initialised on page load. Not a big issue and maybe something we can live with or introduce some js to add and remove the tab index when those are added/removed.

The other issue is when using this on smaller screens. There are some issues with the layout. I know this is a dev tool so maybe something we can live with but some of these should be fairly easy to fix especially the buttons at the bottom. Should be able to add a margin-botton to them, remove the margin-left and add a margin-right. Would be nice to fix the schema select which is overflowing too.

Screenshot 2024-06-11 at 10 39 49

static/javascript/launch.js Outdated Show resolved Hide resolved
templates/launch.html Outdated Show resolved Hide resolved
templates/launch.html Outdated Show resolved Hide resolved
static/javascript/launch.js Outdated Show resolved Hide resolved
@VirajP1002
Copy link
Contributor Author

VirajP1002 commented Jun 12, 2024

Noticed these small issues but also theres a tabbing issue where because of the way you are adding details components when you tab through the inputs you are able to focus on the details components which is confusing because they aren't there yet. This is because there is JS from the DS adding tabindex=0 to those elements when they are initialised on page load. Not a big issue and maybe something we can live with or introduce some js to add and remove the tab index when those are added/removed.

The other issue is when using this on smaller screens. There are some issues with the layout. I know this is a dev tool so maybe something we can live with but some of these should be fairly easy to fix especially the buttons at the bottom. Should be able to add a margin-botton to them, remove the margin-left and add a margin-right. Would be nice to fix the schema select which is overflowing too.

I've seemed to address this issue in both columns, although the size of the required metadata labels are smaller, they don't overflow

@VirajP1002
Copy link
Contributor Author

Worth double checking, it seems like the Flush Survey Data button in your PR stopped working and it opens a schema instead of flushing now?

Yep, a missing value attribute in the button was impacting the logic used in launch.go for flushing, fixed this now 👍

templates/launch.html Outdated Show resolved Hide resolved
Copy link

@Yuyuutsu Yuyuutsu left a comment

Choose a reason for hiding this comment

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

Would it be possible if the Show all button be the same size as the other buttons. More of a personal prefrence. I dont mind if everyone else wants to keep it as it is 👍

@VirajP1002
Copy link
Contributor Author

Would it be possible if the Show all button be the same size as the other buttons. More of a personal prefrence. I dont mind if everyone else wants to keep it as it is 👍

As discussed the text within the button is determines the size of it, happy to change the text to something like Show Metadata. We can see what the other devs think too 👍

@rmccar
Copy link
Contributor

rmccar commented Jun 24, 2024

Screenshot 2024-06-24 at 11 17 42

Theres a bit of extra space added on here when focused on this details component. It only seems to happen on the health or business ones

@VirajP1002
Copy link
Contributor Author

Screenshot 2024-06-24 at 11 17 42

Theres a bit of extra space added on here when focused on this details component. It only seems to happen on the health or business ones

Nice spot, I've address this now, when I was injecting the title in via the JS, it was previously adding in the title within h2 tags, so I just made the JS insert the string instead which fixed the issue 👍

@rmccar
Copy link
Contributor

rmccar commented Jun 25, 2024

Just checked your latest and noticed a pretty minor thing. You have a margin left on the left of these 3 buttons which is pushing them over a bit and arent aligned because of it.

Screenshot 2024-06-25 at 10 52 55

The thing is, If you just remove this it causes this problem:

Screenshot 2024-06-25 at 10 53 08

So what you need to do is if these have a container element add display: flex, flex-wrap: wrap and something like gap: 1rem to it in CSS. Then remove all the margins. This should solve that for you.

Actually scrap that you just need to add the ons-btn-group class to the container

Copy link
Contributor

@liamtoozer liamtoozer left a comment

Choose a reason for hiding this comment

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

Really nice - great work! It looks loads better than before 😄 Just a few very minor cosmetic things I spotted that might end up being fixed by the other comments

Also, it might be to do with how I'm viewing the page in my browser, but things like UUIDs are being truncated for me, would they need to be expanded to fit larger rendered strings?

In required metadata:
Screenshot 2024-06-25 at 14 51 51

In SDS metadata:
Screenshot 2024-06-25 at 14 49 10

static/javascript/launch.js Show resolved Hide resolved
templates/launch.html Outdated Show resolved Hide resolved
static/javascript/launch.js Outdated Show resolved Hide resolved
@rmccar
Copy link
Contributor

rmccar commented Jun 25, 2024

Really nice - great work! It looks loads better than before 😄 Just a few very minor cosmetic things I spotted that might end up being fixed by the other comments

Also, it might be to do with how I'm viewing the page in my browser, but things like UUIDs are being truncated for me, would they need to be expanded to fit larger rendered strings?

In required metadata: Screenshot 2024-06-25 at 14 51 51

In SDS metadata: Screenshot 2024-06-25 at 14 49 10

We have the space to make these a bit wider if we can think of another way of displaying the previous value button. Could try group the buttons with ons-btn-group like my other comment. They should wrap then but it would mean we would have a bigger gap between the inputs when they do.

@VirajP1002
Copy link
Contributor Author

VirajP1002 commented Jun 26, 2024

Really nice - great work! It looks loads better than before 😄 Just a few very minor cosmetic things I spotted that might end up being fixed by the other comments
Also, it might be to do with how I'm viewing the page in my browser, but things like UUIDs are being truncated for me, would they need to be expanded to fit larger rendered strings?
In required metadata: Screenshot 2024-06-25 at 14 51 51
In SDS metadata: Screenshot 2024-06-25 at 14 49 10

We have the space to make these a bit wider if we can think of another way of displaying the previous value button. Could try group the buttons with ons-btn-group like my other comment. They should wrap then but it would mean we would have a bigger gap between the inputs when they do.

Updated this to make the input boxes bigger and match the rest of the box sizes, it involved some reshuffling of a few elements but it makes the result much cleaner.

Screenshot 2024-06-26 at 15 44 02

Copy link
Contributor

@liamtoozer liamtoozer left a comment

Choose a reason for hiding this comment

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

Looks great to me! 👍

static/javascript/launch.js Outdated Show resolved Hide resolved
static/javascript/launch.js Outdated Show resolved Hide resolved
Copy link

@Yuyuutsu Yuyuutsu left a comment

Choose a reason for hiding this comment

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

Looking nice 👍

@VirajP1002 VirajP1002 merged commit 1e389b7 into main Jul 8, 2024
3 checks passed
@VirajP1002 VirajP1002 deleted the use-design-system branch July 8, 2024 07:59
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.

None yet

5 participants