Tidy up non-redoc extension presentation #847

drjonnicholson · 2019-03-14T14:08:30Z

This PR:

Standardises an extension to use the FieldLabel component rather than a bespoke one so the style matches
Removes the x- prefix from the extension when presenting the name on the page
Title cases the unprefixed extension name
Removes the extraneous " quotes from either side of the extension value, so strings like test are displayed as test and not "test"

Before:

(where we're using x-scope to annotate the scope required for a particular field to be populated from our API above and beyond the scope needed to access the endpoint itself, which is defined in security objects etc)

After:

drjonnicholson · 2019-03-14T14:11:07Z

As an aside, I ran npm run prettier before making the commit to make sure it met code standards, and it changed a bunch of files

RomanHotsiy · 2019-03-15T07:16:36Z

src/components/Fields/Extensions.tsx

@@ -1,4 +1,9 @@
 import * as React from 'react';
+
+import { startCase } from 'lodash';


Is there a point to pull in lodash because of the only startCase function?

Could you implement a simple one yourself e.g. in utils or just copy lodash's one

Why are you afraid of importing single function ?
I don't think it is good idea to re-invent (or copy) common functions already defined in libralies.

It's not about importing a single function. It's about adding a new dependency as lodash is not in the dependencies yet.

The main reasoning behind this is the bundle size. While it doesn't matter so much for node apps it matters for browsers. Tree-shaking isn't perfect. I just tested and importing this single function leads to 68KB! increase in bundle size. Don't you find it too much for such a simple function (especially since vendor extension names can't contain any special Unicode symbols).

Hope this does make sense.

It makes sense 👍
Looks like lodash are not used in production code right now, so importing single function may add 68KB or around to bundle.

Perhaps lodash dependencies should be replaced with more granular modules like lodash.set.

Yes, but this is development code so it doesn't matter so much and I don't think it's worth to do.

Hi @RomanGotsiy and @exoego, thanks for having a look at this.

So I agree with @exoego to avoid reinventing the wheel etc where possible, but 68k bundle increase is too big for a quite simple function. I'm ultimately happy with switching this out to something more appropriate. Happy with what you guys decide really.

I'm not sure now if we need to title-case the extension name at all.

Vendor extension can contain arbitrary name so e.g.: x-Some Title is a valid extension. So you have a workaround to show title-cased labels but if we title case them by default other users don't have a way to display lower-case label.

Summing up, I think we should remove startCase at all. Do you agree?

Fair point, if x-Some Title is valid then yeah completely agree - we can't guess or impose one method of presentation over another.

And I can solve my use case by just capitalising the first letter after the x- prefix, so I'm happy.

I'll update the PR in a moment

RomanHotsiy · 2019-03-15T07:17:17Z

Also, add a screenshot of how it looks like now, please! That way it would be much easier for me.

Thanks in advance!

drjonnicholson · 2019-03-15T10:27:59Z

@RomanGotsiy I've updated the PR description with before and after images

RomanHotsiy · 2019-03-15T10:41:13Z

Thanks! While you're already on it. Could you plz also check text vertical alignment issue (check on the screenshot) 🙇

If you can't fix it in 10-15 mins, leave it for me!

coveralls · 2019-03-15T10:47:21Z

Coverage increased (+0.03%) to 82.563% when pulling 44b3512 on drjonnicholson:master into 178ff4c on Rebilly:master.

drjonnicholson · 2019-03-15T11:01:46Z

Good spot, so looking at default/exmaples, the : is inside the FieldLabel component, so that solves the vertical alignment issue.

There is also a horizontal issue in the value being closer to the label than it is to say an example. Here's a mocked up example:

I could either:

Leave as is
Reuse the existing ExampleValue styled component
Duplicate the ExampleValue component as ExtensionValue in field.ts, giving us a new extension hook etc.

Personally I like the duplication in this case - since it's initially duplicated but can be modified independently of examples etc, which serve a different purpose

I'm also tempted to remove the opacity of the Extension component. That sound good?

Any other changes while I'm at it? 😃

RomanHotsiy · 2019-03-15T11:13:21Z

Personally I like the duplication in this case - since it's initially duplicated but can be modified independently of examples etc, which serve a different purpose

Can you try extending just extending it? i.e.

export const ExtensionValue = styled(ExampleValue)``

No extra extension hook though but it can be added later if needed.

I'm also tempted to remove the opacity of the Extension component. That sound good?

Yes, go ahead!

drjonnicholson · 2019-03-15T11:16:44Z

Yeah that makes sense. Next commit going up, let me know if there's anything else under this PR you want done.

drjonnicholson · 2019-03-15T11:39:09Z

Think that's everything. Output of Example against and Extension now looks like mock up (just tweaking he DOM a little to put them next to each other with same label value of Example:):

RomanHotsiy · 2019-03-15T15:15:09Z

Looks awesome now! Thanks! 🙇

RomanHotsiy · 2019-03-15T15:21:01Z

Just refactored it a bit dc77d8f.

Check it out please just in case.

drjonnicholson · 2019-03-16T17:51:10Z

Nice change. I imagine Inspecting the type must be faster than string manipulation. Feel bad I didn’t think of that approach!

Thanks for making a great product all, and for making submitting improvements a welcoming experience 🙂

* master: (73 commits) chore: fixes typo in error message for detecting a circular dependency in theme.ts (Redocly#852) chore: minor refactor fix: tidy up non-redoc vendor extension presentation (Redocly#847) chore(cli): redoc-cli v0.8.3 fix(cli): add node-libs-browser to the deps chore(cli): redoc-cli v0.8.2 fix: fix redoc-cli broken dependencies chore: Release 2.0.0-rc.4 🔖 feat: display requestBody description Redocly#833 (Redocly#838) fix: move swagger2openapi to deps because of missing transitive deps chore(cli): redoc-cli v0.8.0 chore: update peerDeps before release chore: Release 2.0.0-rc.3 🔖 fix: add extra deref step for anyOf/oneOf variants chore: Remove duplicate re-export from index.ts (Redocly#842) fix: pin lunr version in ReDoc chore: fix cli declarations feat: support externalValue for examples chore: update deps docs: Fixed docker README, added missing redocly repo name (Redocly#841) ...

Tidy up non-redoc extension presentation

cb86e41

RomanHotsiy requested changes Mar 15, 2019

View reviewed changes

Remove importing code from Lodash

e8071b4

Move colon

6f48090

drjonnicholson added 4 commits March 15, 2019 11:18

Remove opacity, grab ExtensionValue from fields.ts

ed8dc62

Linting

9fd7617

Linting

36b0045

Add spacing as in FieldDetail

44b3512

RomanHotsiy approved these changes Mar 15, 2019

View reviewed changes

RomanHotsiy merged commit b21cd3d into Redocly:master Mar 15, 2019

drjonnicholson mentioned this pull request Mar 19, 2019

Clean up how field values are displayed #855

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tidy up non-redoc extension presentation #847

Tidy up non-redoc extension presentation #847

drjonnicholson commented Mar 14, 2019 •

edited

Loading

drjonnicholson commented Mar 14, 2019

RomanHotsiy Mar 15, 2019

exoego Mar 15, 2019

RomanHotsiy Mar 15, 2019 •

edited

Loading

exoego Mar 15, 2019 •

edited

Loading

RomanHotsiy Mar 15, 2019

drjonnicholson Mar 15, 2019

RomanHotsiy Mar 15, 2019

drjonnicholson Mar 15, 2019

RomanHotsiy commented Mar 15, 2019

drjonnicholson commented Mar 15, 2019

RomanHotsiy commented Mar 15, 2019 •

edited

Loading

coveralls commented Mar 15, 2019 •

edited

Loading

drjonnicholson commented Mar 15, 2019

RomanHotsiy commented Mar 15, 2019 •

edited

Loading

drjonnicholson commented Mar 15, 2019

drjonnicholson commented Mar 15, 2019

RomanHotsiy commented Mar 15, 2019

RomanHotsiy commented Mar 15, 2019

drjonnicholson commented Mar 16, 2019

		@@ -1,4 +1,9 @@
		import * as React from 'react';

		import { startCase } from 'lodash';

Tidy up non-redoc extension presentation #847

Tidy up non-redoc extension presentation #847

Conversation

drjonnicholson commented Mar 14, 2019 • edited Loading

drjonnicholson commented Mar 14, 2019

RomanHotsiy Mar 15, 2019

Choose a reason for hiding this comment

exoego Mar 15, 2019

Choose a reason for hiding this comment

RomanHotsiy Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

exoego Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

RomanHotsiy Mar 15, 2019

Choose a reason for hiding this comment

drjonnicholson Mar 15, 2019

Choose a reason for hiding this comment

RomanHotsiy Mar 15, 2019

Choose a reason for hiding this comment

drjonnicholson Mar 15, 2019

Choose a reason for hiding this comment

RomanHotsiy commented Mar 15, 2019

drjonnicholson commented Mar 15, 2019

RomanHotsiy commented Mar 15, 2019 • edited Loading

coveralls commented Mar 15, 2019 • edited Loading

drjonnicholson commented Mar 15, 2019

RomanHotsiy commented Mar 15, 2019 • edited Loading

drjonnicholson commented Mar 15, 2019

drjonnicholson commented Mar 15, 2019

RomanHotsiy commented Mar 15, 2019

RomanHotsiy commented Mar 15, 2019

drjonnicholson commented Mar 16, 2019

drjonnicholson commented Mar 14, 2019 •

edited

Loading

RomanHotsiy Mar 15, 2019 •

edited

Loading

exoego Mar 15, 2019 •

edited

Loading

RomanHotsiy commented Mar 15, 2019 •

edited

Loading

coveralls commented Mar 15, 2019 •

edited

Loading

RomanHotsiy commented Mar 15, 2019 •

edited

Loading