-
-
Notifications
You must be signed in to change notification settings - Fork 135
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: Dimension switcher #323
Conversation
dimensionName={dimensionName} | ||
icon={dimensionConfiguration.get('icon')} | ||
dimensionLabel={dimensionConfiguration.get('label')} | ||
presetLabel={activePresets.get(dimensionName).get('label')} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here I'd rather use Plow.js, like $get([dimensionName, 'label'], activePresets)
-- so to not couple the GET to Immutable.JS.
Hey @hlubek, I just checked your code, generally 👍 really nice :) -just a few nitpicks. If you like, we can also merge intermediate states, as the feature does not exist at all so far. All the best, |
@skurfuerst Hey, but why are tests failing again? |
Good question - we need to check this! |
3d76a62
to
9e73849
Compare
@dimaip Agreed, I'll check the test and lint and refactor a bit (including @skurfuerst comments) when the feature works basically. |
- It's more reliable to use absolute node URIs - Resolving shortcuts is misleading for node information
Yippie, it's working! There are some improvements we could implement:
|
new Map({ | ||
byName: Immutable.fromJS(byName(state)), | ||
active: Immutable.fromJS(active(state)), | ||
allowedPresets: Immutable.fromJS(allowedPresets(state)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, I'd like to have some documentation how the structure in the state actually looks like :-) As that is crucial to understand for the other parts
Hey Christopher @hlubek - awesome change :) Just two nitpicks; once they are solved, feel free to merge the change 👍 All the best, |
Merging with @skurfuerst's permission 😄 |
metaData
for now, could be improved to use an API service and async update via saga)