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

Sprint22 staging #458

Merged
merged 32 commits into from
Nov 1, 2021
Merged

Sprint22 staging #458

merged 32 commits into from
Nov 1, 2021

Conversation

kaladay
Copy link
Contributor

@kaladay kaladay commented Oct 25, 2021

Resolves:

kaladay and others added 29 commits October 12, 2021 16:19
Use the manifest from the state.

The tests have some problems that are exposed by this change.
Therefore, the tests have been rewritten.
The re-write attempts to stay true to the original design where reasonable.

The tests were passing but were not passing for the correct reasons.
Some of the problems:
- The tests are being configured without a beforeEach, possibly sharing the same data between tests.
- The settings are added throughout the file rather than in a consistent manner.
- The tests are testing against non-existent array indexes on an object (resulting in undefined === undefined tests that pass).
- There is code being executed outside of tests that are used inside of tests (and are not inside a beforeEach).
The attributes `active-tab-theme` and `inactive-tab-theme` for specifying weaver colors, such as "success" or "warning".
The attributes may be set globally on the `<wvre-tabs>` or locally on each `<template>` inside of the `<wvre-tabs>`.
Issue 439: Incorrect manifest entries are being updated in Manifest.
Fix boolean logic and related tests.

The test variable `request` assignment is setting the entryName to a non-existent name by default.
After fixing this in the tests, it exposed some additional problems that are now fixed.
…eming

Issue 348: The tabs component should support theming.
…ests

Issue 442: Incorrect boolean logic in dequeue and inadequate tests.
…icker due to loop involving scrollbar.

The `this.isSticky !== newIsSticky` is resulting in the `this.isSticky` being toggled.
The `newIsSticky` is being changed when the width changes because of several reasons, each being independent problems:
1) On systems where the parent has a height of `100%`, the `compareHeight` is always the window height.
2) When `this.isSticky` is true and the height of the footer suddenly changes (such as a login button dropping below).
3) The calculation is (conditionally) subtracting the footer height from the window height.
4) The sticky height and the non-sticky height are different.

The solution is to:
1) Save the state of the total height of the parent for when the footer is not sticky (or best available dimensions).
2) Always calculate against the height as if it were not sticky, regardless of the state of `this.isSticky`.
3) What matters is the relationship between the window height and the total height, compare against that only.
4) Because this is potentially being called a lot, be pickier about boolean logic to reduce performance concerns.
5) The `@debounce` and a lock strategies have proven to be problematic in this case.*
6) I am unsure if I should use `<=` or `>=` (as opposed to `<` or `>`), I have opted to only change sticky when necessary (therefore using `<` and `>`).

*) In the case of a lock, the lock works except that it should not lock the clientWidth assignment.
As this change is outside the scope of this bug fix, I have withheld the lock approach.
I suggest future changes to consider a lock (but not a debounce) that does not include locking out the width adjustment.
This is a bit hackish and as such comments are added as appropriate.
It would be preferable for a better solution to be provided but for now this should be acceptable.
Issue 369: In some browsers at some sizes, the footer will shudder/flicker due to loop involving scrollbar.
Issue 445: Weaver Card Should Support Expand/Collapse on Card Header click.
…ent.

Add stompjs and sockjs-client dependencies.

Implement StompManifest and StompClient.
This implementation is based off of the Manifest.

A stomp-client-protocol is provided with `LONG_POLLING`, `WEB_SOCKET`, and `SERVER_SIDE_EFFECT` but only `WEB_SOCKET` is guaranteed at this time.
More work may be necessary to get the other two to work.

The feature requests "Message Receiver Component" but there is initial structure for sending messages.
This is not implemented at this time and only the "Receiver" part of the component is provided.

Additional work to manage unsubscription on disconnect may be needed for proper maintenance and cleanup tasks.

There are a few mapping strategies provided:
- `WEAVER`
- `JSONPARSE`
- `NONE`

The `WEABER` will deconstruct the weaver message structure and re-map it to a more flexible json structure.
The `JSONPARSE` will directly parse the string into json structure without any alterations.
The `NONE` should pass the response through as a string.

This (in theory) supports multiple manifests.
This supports multiple manifest entries via a single manifest.
This supports custom mapping-strategies per entry as well as a mapping-strategy global to the manifest.

Due to time constraints, only minimal tests are added.

Example Generic usage:
```
  <wvre-stomp-manifest name="Wvr Stomp Manifest" broker-url="http://localhost:8181/ws" mapping-strategy="jsonparse" protocol="WEB_SOCKET">
    <wvre-stomp-manifest-entry name="Echo" destination="/echo"></wvre-stomp-manifest-entry>
    <wvre-stomp-manifest-entry name="Board" destination="/board"></wvre-stomp-manifest-entry>
  </wvre-stomp-manifest>
```
This moves the width into the scss.
Swap the fixed detection logic.
[MERGER] Issue 451: Weaver Card hardcodes width as a style.
Issue 369: Logic appears to be reversed.
Use a more generic name for better public design.
The current design favors STOMP but this need not be so.
Should any future changes involve supporting more, the structure can still remain as "message" while the functionality can change.
…a element attribute `collapsed`.

The `collapsed` element attribute is a `true`/`false` boolean value that will update when either the internal boolean state changes or the element attribute value is manually changed.
… 'false'.

The previous styling is not consistent with our practices.
The consistent way is apparently prohibited and so this is a compromise to get as close to the preferred practices.
…e-programmatically

Issue 445: Enable programmatic control over expand/collapsed state via element attribute `collapsed`.
…hashed out yet.

Remove these disable commands as soon as a testing strategy is determined and when tests can be written.
…mponent

Issue 407: Weaver Components should provide a Message Receiver Component.
@kaladay kaladay requested a review from jcreel October 25, 2021 14:21
ghost
ghost previously approved these changes Oct 26, 2021
@jeremythuff jeremythuff merged commit 05846d9 into master Nov 1, 2021
@kaladay kaladay deleted the sprint22-staging branch November 19, 2021 20:23
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

3 participants