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

[OCTANE] change ember-core-concepts.png to use </> syntax #631

Merged
merged 7 commits into from
May 30, 2019

Conversation

muziejus
Copy link
Contributor

The original png used curlies for a component. I've changed that here. I've also made an editable svg out of the file, so hopefully it'll be easier to update in the future

that said, I think this image can be made better. I think it's still confusing:

  • why does findAll lead to an arrow going to the Model but not coming back?
  • why does findAll talk about persisting to the web server; no changes have been made
  • It's not clear that the route handler loads a template

etc. I can try to make a few changes to try and make it a bit clearer, but I want this minimum change in as a PR.

@muziejus
Copy link
Contributor Author

Also, this version uses "free" fonts (Roboto and Source Code Pro), but it'd probably be best if the svg inherited the css from the site proper.

@rwjblue
Copy link
Contributor

rwjblue commented Mar 17, 2019

Would you mind sharing before/after screenshots too?

@Frozenfire92
Copy link
Contributor

I love the switch to an svg in regards to maintainability. However by using non standard fonts, there is potential for it not to be consistent on every screen. For example this is how it looks for me

Screen Shot 2019-03-17 at 2 13 30 PM

I suspect we also want to update this code to be in the new format

import Route from '@ember/routing/route';

export default class RentalsRoute extends Route {
  model() {
    return this.store.findAll('rental');
  }
}
<RentalTile @rental={{rental-unit}} />

@muziejus
Copy link
Contributor Author

@Frozenfire92: I agree. I think it should adopt the fonts used in the css for the guides. I haven't yet done that. Let me try that now.

@muziejus
Copy link
Contributor Author

As for the native class syntax on the Route, I left it as is because the octane guides so far also don't have native class syntax in Routing. See, for example,

https://github.com/ember-learn/guides-source/blob/octane/guides/release/routing/specifying-a-routes-model.md

@Frozenfire92
Copy link
Contributor

looking at the Routing section in #588 nothing is completed. I suspect class syntax is desired, but would like confirmation from ember core team :)

@muziejus
Copy link
Contributor Author

That was my thinking too!

I'm working on the text spacing stuff now to see if I can get it to play nicely w/ different fonts.

@muziejus muziejus changed the title [OCTANE] change ember-core-concepts.png to use </> syntax [WIP] [OCTANE] change ember-core-concepts.png to use </> syntax Mar 17, 2019
@muziejus
Copy link
Contributor Author

OK, here are the before and after, now that I've cleaned up the svg somewhat.
Screenshot 2019-03-17 at 14 39 10
Screenshot 2019-03-17 at 14 39 51

@Frozenfire92
Copy link
Contributor

Noting this looks ok on Chrome and Safari on OSX but looks weird in Firefox
Screen Shot 2019-03-17 at 4 25 29 PM

@muziejus
Copy link
Contributor Author

Screenshot 2019-03-17 at 20 23 27

I added text anchors for the first lines of the code, so hopefully that should fix the firefox problem (this is how the image looks in firefox to me).

@muziejus muziejus changed the title [WIP] [OCTANE] change ember-core-concepts.png to use </> syntax [OCTANE] change ember-core-concepts.png to use </> syntax Mar 18, 2019
@muziejus
Copy link
Contributor Author

OK I took away WIP since I think this is ready for review?

@mixonic
Copy link
Contributor

mixonic commented Mar 18, 2019

@muziejus thanks for this work! Can you also make the variable in the template rentalUnit instead of rental-unit? I'm not sure why the latter is being suggested, we commonly use javaScriptStandardCamelCase for variables in templates.

@Frozenfire92
Copy link
Contributor

Didn't mean to suggest that, just was fixing up the already there example

100% right though, should be rentalUnit

@muziejus
Copy link
Contributor Author

Good catch, @mixonic . As @Frozenfire92 suggested, that was already in the original. I was more reacting to the lack of angle bracket syntax and completely overlooked the kebab-case!

@jenweber
Copy link
Contributor

This is such an improvement! Thanks @muziejus for your patience with the back & forth. I'm merging this, and if there are any other outstanding changes people were looking for (browser compatibility), let's hit those in separate PRs.

Nice work!

@jenweber jenweber merged commit d5fc7ca into ember-learn:octane May 30, 2019
@ghost ghost mentioned this pull request Feb 19, 2020
lenoraporter pushed a commit to lenoraporter/guides-source that referenced this pull request Jul 19, 2020
…learn#631)

* Replace core-concepts image w/ svg that includes angle bracket syntax

* Components, not templates, in the bottom row.

* Fix fi ligature problem.

* Change fonts to match css of Ember guides.

* Clean up classnames and code section.

* Add text anchors for first lines.

* Change template variable to camelCase
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.

5 participants