ui: Toggle for `read-only` view #16279

ChaiWithJai · 2023-02-28T22:02:37Z

Closes #16269 for creating a Toggle Button to display both the Job Specification and Full Definition.

Functional Changes
This PR adds a toggle button when the JobEditor component is in a read-only state.

Refactoring
This PR refactors the JobEditor component to improve its flexibility and maintainability. The original code tightly coupled the component's state with its rendering logic, which made it difficult to modify and extend. This PR introduces several improvements to make the code more modular and easier to understand.

Changes

Uses Ember's component helper to dynamically render the appropriate child component based on the value of the stage property.
Reorganizes the code to separate the component state from the rendering logic, which makes the component more modular and easier to test.

github-actions · 2023-02-28T22:10:51Z

Ember Asset Size action

As of 0cefe88

Files that got Bigger 🚨:

File	raw	gzip
nomad-ui.js	+3.49 kB	+680 B
nomad-ui.css	+386 B	+57 B

Files that stayed the same size 🤷‍:

File	raw	gzip
vendor.js	0 B	0 B
vendor.css	0 B	0 B

github-actions · 2023-02-28T22:18:43Z

Ember Test Audit comparison

	15982/trunk/main	`0cefe88`	change
passes	1467	1464	-3
failures	0	4	+4
flaky	0	0	0
duration	11m 20s 220ms	000ms	-11m 20s 220ms

ChaiWithJai · 2023-03-01T19:56:43Z

ui/app/templates/components/job-editor/read.hbs

+    <div class="pull-right" style="display: flex">
+        <div class="select-mode" data-test-toggle={{@data.view}}>
+        <button 
+            class="button is-small is-borderless {{if (eq @data.view "job-spec") "is-active"}}"
+            type="button"
+            {{on "click" @fns.onToggle}}
+        >
+            Job Spec
+        </button>
+        <button 
+            class="button is-small is-borderless {{if (eq @data.view "full-definition") "is-active"}}"
+            type="button"
+            {{on "click" @fns.onToggle}}
+            data-test-toggle-full
+        >
+            Full Definition
+        </button>
+        </div>


This should be conditionally rendered because old jobs will not have this functionality. This wasn't addressed in the mocks so I will tackle this issue separately.

ChaiWithJai

FYI it allows users to toggle between full specification and JSON definition is a test that is made to fail until the next PR.

philrenaud · 2023-03-01T20:09:54Z

Small note: if you use the GitHub web ui's "Create a branch for...." from an issue, it'll auto-track it. Makes it a little easier to traverse from issue to pr and back.

philrenaud

A few changes and requests for clarification.

Some of the terminology could be changed to be clearer IMO. For example, I'm able to grok that definition means JSON and specification means HCL in this context, but that wasn't immediately clear.

ui/app/templates/components/job-editor/edit.hbs

ui/app/components/job-editor.js

philrenaud · 2023-03-01T20:18:25Z

ui/app/routes/jobs/job/definition.js

+    const definitionWithoutSpecification = { ...definition };
+    delete definitionWithoutSpecification.Specification;
+
+    const specification = await new Blob([
+      definition?.Specification?.Definition,
+    ]).text();


Can you help me understand what this is accomplishing? delete prop outside of a serializer has me wondering if this is the optimal place for this.

fetchRawDefinition specifically is made to skip the Ember Data and thus should not be serialized because we serialize payloads to format them before they enter the Ember Data store.

That leaves me with 2 places to handle this concerns: the Route who makes the fetch request outside of Ember Data or creating a Service and extract our logic there. I opted for minimally changing the API surface and handling the concern where we are loading the data.

Okay, so why are we deleting .Specification here?

What's the format of definition ? Why does Specification need to be removed from it? I'm struggling to understand what this model is trying to accomplish.

We're doing what we would do in our Serializer here. We don't require this field. We could keep it, as we're not concerned with it and it won't break anything.

philrenaud · 2023-03-01T20:20:47Z

ui/tests/acceptance/job-definition-test.js

+    },
+  },
+};
+


Q: why not use a factory to accomplish this? They're extensible and most things you have listed here are either already job defaults or else unrelated to the acceptance test below.

I'm not in the camp of factory or writing test logic. It becomes painful to maintain and that's personally the part of the test suite I hate the most. If you want it, I'm happy to create it. But it's just more code to maintain.

There's a few things here:

This data isn't going to be modeled because this is a raw request. We're explicitly using the API response to visualize the response itself in code mirror.

We could overcome this using the afterCreate hook. But we already have 200 lines of logic in that hook and I simply don't want to touch it.

The best solution would be to just move this into a utility file if you wish to separate this fixture from the test code. And I can address that in a follow up PR.

ui/app/styles/components/codemirror.scss

ui/app/components/job-editor.js

Co-authored-by: Phil Renaud <[email protected]>

* chore: update mirage scenario: * ui: conditionally render toggle button (#16497)

ChaiWithJai

I noticed there were test failures associated with the job definition acceptance test module. However, those test failures don't occur for me locally. I will ignore the test failures for now for that reason.

* ui: model update for specification * style: add styling for select * style: add styling for select * refact: add spec to view * refact: update component API * test: refactor for new UI state * refact: clean conditional * refact: update component API for prop * chore: correct naming * chore: remove `fn` helper Co-authored-by: Phil Renaud <[email protected]> * update `default` Mirage scenario (#16496) * chore: update mirage scenario: * ui: conditionally render toggle button (#16497) * chore: update css variable name (#16498) --------- Co-authored-by: Phil Renaud <[email protected]>

* ui: Toggle for `read-only` view (#16279) * ui: model update for specification * style: add styling for select * style: add styling for select * refact: add spec to view * refact: update component API * test: refactor for new UI state * refact: clean conditional * refact: update component API for prop * chore: correct naming * chore: remove `fn` helper Co-authored-by: Phil Renaud <[email protected]> * update `default` Mirage scenario (#16496) * chore: update mirage scenario: * ui: conditionally render toggle button (#16497) * chore: update css variable name (#16498) --------- Co-authored-by: Phil Renaud <[email protected]> * refact: `JobEditor` reactive query parameters (#16710) * refact: add query parameter * refact: move toggle action to controller * ui: remove toggle behavior in `JobEditor` (#16711) * refact: rename logic for select * chore: instantiate qp in route * refact: uniform alerts (#16715) * style: buffer between alert and header * refact: extract alerts into a component * chore: update tests for qp * chore: defensive logic for app controller * refact: move `edit` state to controller (#16725) * refact: move edit state to controller * refact: handle edit state (#16731) * refact: handle edit state * ui: warning message (#16732) * ui: warning message * ui: enable editing of HCL vars in the UI (#16734) * enable editing of HCL vars * refact: default qp logic * refact: alert condition * refact: variables as string * style: revert styling change --------- Co-authored-by: Phil Renaud <[email protected]>

…ition` view (#16669) * ui: Toggle for `read-only` view (#16279) * ui: model update for specification * style: add styling for select * style: add styling for select * refact: add spec to view * refact: update component API * test: refactor for new UI state * refact: clean conditional * refact: update component API for prop * chore: correct naming * chore: remove `fn` helper Co-authored-by: Phil Renaud <[email protected]> * update `default` Mirage scenario (#16496) * chore: update mirage scenario: * ui: conditionally render toggle button (#16497) * chore: update css variable name (#16498) --------- Co-authored-by: Phil Renaud <[email protected]> * ui: Display JSON view of variables associated to job specification (#16570) * chore: move fixture to util * chore: update tests: * ui: display variables table * chore: add mirage fixture (#16572) * ui: regex for job spec parse (#16668) * ui: remove variable table (#16670) * ui: notify user if specification has variables (#16671) * ui: regex for job spec parse * chore: deprecate variable references * chore: update mirage * ui: add notification * test: add test coverage for parse method (#16590) * refact: `JobEditor` reactive query parameters (#16710) * refact: add query parameter * refact: move toggle action to controller * ui: remove toggle behavior in `JobEditor` (#16711) * refact: rename logic for select * chore: instantiate qp in route * refact: uniform alerts (#16715) * style: buffer between alert and header * refact: extract alerts into a component * chore: update tests for qp * chore: defensive logic for app controller * refact: move `edit` state to controller (#16725) * refact: move edit state to controller * refact: handle edit state (#16731) * refact: handle edit state * ui: warning message (#16732) * ui: warning message * ui: enable editing of HCL vars in the UI (#16734) * enable editing of HCL vars * refact: default qp logic * refact: alert condition * refact: Pass `variables` as string (#16849) * ui: Toggle for `read-only` view (#16279) * ui: model update for specification * style: add styling for select * style: add styling for select * refact: add spec to view * refact: update component API * test: refactor for new UI state * refact: clean conditional * refact: update component API for prop * chore: correct naming * chore: remove `fn` helper Co-authored-by: Phil Renaud <[email protected]> * update `default` Mirage scenario (#16496) * chore: update mirage scenario: * ui: conditionally render toggle button (#16497) * chore: update css variable name (#16498) --------- Co-authored-by: Phil Renaud <[email protected]> * refact: `JobEditor` reactive query parameters (#16710) * refact: add query parameter * refact: move toggle action to controller * ui: remove toggle behavior in `JobEditor` (#16711) * refact: rename logic for select * chore: instantiate qp in route * refact: uniform alerts (#16715) * style: buffer between alert and header * refact: extract alerts into a component * chore: update tests for qp * chore: defensive logic for app controller * refact: move `edit` state to controller (#16725) * refact: move edit state to controller * refact: handle edit state (#16731) * refact: handle edit state * ui: warning message (#16732) * ui: warning message * ui: enable editing of HCL vars in the UI (#16734) * enable editing of HCL vars * refact: default qp logic * refact: alert condition * refact: variables as string * style: revert styling change --------- Co-authored-by: Phil Renaud <[email protected]> * bug: correctly edit variables (#16989) * ui: visualize variables (#16987) * ui: fetchRawSpecification * refact: integrate new model method * test: fetchRaw unit * styling: enable height on cm * chore: update copy * feat: visual variables * chore: conditional render info txt * refact: add mirage endpoint * refact: update test for new schema * refact: job submit flow (#17015) * refact: job update logic * chore: remove dead code * bug: update `job.run` and `job.update` adapter methods (#17055) * refact: update adapter * chore: update api usage * styling: UX requests (#17064) * refact: update adapter * chore: update api usage * styling: disable toggle w text * styling: stick button * style: space out alerts * chore: autofocus on first editor * bug: dismiss alert * chore: add jsdoc and assertion check * chore: update mirage for Vercel (#17054) * chore: mirage logic for vercel deploy * chore: update test for mirage change * refact: API refactoring (#17083) * refact: udpate for req schema * refact: update for variable flags and literal * bug: visualize job model not derived state * chore: update copy * chore: fix incorrect copy * chore: deprecate variables derived state * chore: update copy * feat: enable toggle on edit * chore: prettify * refact: move conditional --------- Co-authored-by: Phil Renaud <[email protected]>

ChaiWithJai added 7 commits February 28, 2023 16:56

ui: model update for specification

97ebc64

style: add styling for select

7bc2b71

style: add styling for select

85448d2

refact: add spec to view

b946579

refact: update component API

833582d

test: refactor for new UI state

0709b3c

refact: clean conditional

b4cb7f4

ChaiWithJai changed the title ~~ui: Display HCL in the UI~~ ui: Toggle for read-only view Feb 28, 2023

ChaiWithJai added 2 commits March 1, 2023 14:44

refact: update component API for prop

b508110

chore: correct naming

27b8b01

vercel bot deployed to Preview – nomad-storybook-and-ui March 1, 2023 19:51 View deployment

ChaiWithJai commented Mar 1, 2023

View reviewed changes

ChaiWithJai requested a review from philrenaud March 1, 2023 19:57

ChaiWithJai commented Mar 1, 2023

View reviewed changes

ChaiWithJai marked this pull request as ready for review March 1, 2023 19:58

philrenaud requested changes Mar 1, 2023

View reviewed changes

chore: remove fn helper

8717387

Co-authored-by: Phil Renaud <[email protected]>

vercel bot deployed to Preview – nomad-storybook-and-ui March 1, 2023 21:20 View deployment

update default Mirage scenario (#16496)

af089b0

* chore: update mirage scenario: * ui: conditionally render toggle button (#16497)

vercel bot deployed to Preview – nomad-storybook-and-ui March 15, 2023 16:01 View deployment

chore: update css variable name (#16498)

0cefe88

ChaiWithJai requested a review from philrenaud March 15, 2023 16:04

vercel bot deployed to Preview – nomad-storybook-and-ui March 15, 2023 16:08 View deployment

ChaiWithJai commented Mar 16, 2023

View reviewed changes

philrenaud approved these changes Mar 17, 2023

View reviewed changes

ChaiWithJai merged this pull request into 15982/trunk/main Mar 20, 2023

ChaiWithJai deleted the 15982/trunk/hcl-in-ui branch March 20, 2023 16:25

tgross mentioned this pull request Jun 28, 2024

[ui, hcl] Toggle Button #16269

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: Toggle for `read-only` view #16279

ui: Toggle for `read-only` view #16279

ChaiWithJai commented Feb 28, 2023

github-actions bot commented Feb 28, 2023 •

edited

Loading

github-actions bot commented Feb 28, 2023 •

edited

Loading

ChaiWithJai Mar 1, 2023

ChaiWithJai left a comment

philrenaud commented Mar 1, 2023

philrenaud left a comment

philrenaud Mar 1, 2023

ChaiWithJai Mar 1, 2023

philrenaud Mar 1, 2023

ChaiWithJai Mar 1, 2023

philrenaud Mar 1, 2023

ChaiWithJai Mar 1, 2023

ChaiWithJai Mar 16, 2023

ChaiWithJai left a comment

ui: Toggle for read-only view #16279

ui: Toggle for read-only view #16279

Conversation

ChaiWithJai commented Feb 28, 2023

github-actions bot commented Feb 28, 2023 • edited Loading

Ember Asset Size action

github-actions bot commented Feb 28, 2023 • edited Loading

Ember Test Audit comparison

Choose a reason for hiding this comment

ChaiWithJai left a comment

Choose a reason for hiding this comment

philrenaud commented Mar 1, 2023

philrenaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChaiWithJai left a comment

Choose a reason for hiding this comment

ui: Toggle for `read-only` view #16279

ui: Toggle for `read-only` view #16279

github-actions bot commented Feb 28, 2023 •

edited

Loading

github-actions bot commented Feb 28, 2023 •

edited

Loading